Skip to content

keepalived: Added track_script option to sync group and fixed print_track_script_indent#29027

Open
rishabhshah2005 wants to merge 8 commits intoopenwrt:masterfrom
rishabhshah2005:master
Open

keepalived: Added track_script option to sync group and fixed print_track_script_indent#29027
rishabhshah2005 wants to merge 8 commits intoopenwrt:masterfrom
rishabhshah2005:master

Conversation

@rishabhshah2005
Copy link
Copy Markdown

@rishabhshah2005 rishabhshah2005 commented Mar 30, 2026

📦 Package Details

Maintainer: @feckert
keepalived: Added track_script section to sync group and fixed print_track_script_indent

Description:

  1. Added track_script section to sync_group (only vrrp_script can be added)
  2. Fixed issue where track_script section was not being added in instance
  3. updated the config file and added some documentation

Build Test
Build tested successfully on a x86_64 device

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

@feckert
Copy link
Copy Markdown
Member

feckert commented Mar 31, 2026

  • Please use <= 60 chars per head line in the commit
  • Please use <= 75 chars per body line in the commit

Just have a look at the other commits in this repository – that’s a good place to start.

@feckert feckert requested review from Copilot and feckert March 31, 2026 05:48
@rishabhshah2005
Copy link
Copy Markdown
Author

rishabhshah2005 commented Mar 31, 2026

You want me to change commit messages in this PR itself?

@feckert
Copy link
Copy Markdown
Member

feckert commented Mar 31, 2026

You want me to change commit messages in this PR itself?

No. I mean in the commits. The commits get merged into the repository not the PR description.

@rishabhshah2005
Copy link
Copy Markdown
Author

rishabhshah2005 commented Mar 31, 2026

I meant the commit messages. How can I can I go back and change the messages. Sorry i am new to this and i have no idea how you guys would approach to do this

Edit: Never mind I did it. Are the changes acceptable now?

This function was not correctly parsing track_script.
Removed option value as it was of no use and replaced
it with existing option name.
Removed check where the section and option name
had to be equal

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
track_script was not present for sync_group.
note that with sync group, option priority doesnt
work with vrrp_script

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
added a bit of documentation for vrrp_script for the missing options
added new option for vrrp_sync_group

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
Added option timeout in vrrp_script section
Made corresponding changes to keepalived.config as well

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
In the section handling the parsing of track script for vrrp_instance,
changed curr_track_elem to vrrp_script

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
Copy link
Copy Markdown
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s always the first time 👍

There are my comments

[ -z "$group" ] && return 0

# Check if we have 'vrrp_instance's defined for
# Check if we have 'vrrp_instance's defined for
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not belongs to this change.
Either new commit or leave it as it is.

procd_set_param respawn
procd_close_instance
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not belongs to this change.
Either new commit or leave it as it is.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant this be accepted? Or should make changes in another commit to leave it as it is?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said. If this is not related to the change move it to a new commit.
I this case for example keepalived: remove empty line or in the other case keepalived: remove trailing whitespace

# list group "VI_2"
# option name "VI_sync_group_1"
# list group "VI_1"
# list group "VI_2"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes

#	option name		  "VI_sync_group_1"
#	list group		  "VI_1"
#	list group		  "VI_2"

are not needed leave it as it is

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some documentation for the new option here. Is the documentation not needed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s fine that this is documented. But you’ve changed lines that you didn’t need to change.

local track_script_val
config_get track_script_val "$1" track_script
if [ -n "$track_script_val" ]; then
printf '%btrack_script {\n' "${INDENT_1}" >> "$KEEPALIVED_CONF"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use config_section_open there?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use config_section_open here but it would mess up the indentation in keepalived.conf. Besides for this is referenced referenced the track_script logic in vrrp_instance

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should extend the config_section_open and config_section_close function, to add a new indent function argument. If argument is not set, proceed as use the default indent as before, but if the argument indent is set (INDENT_1|INDENT_2 ...), then add this indent.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah we can do that. But after this should all the functions that hardcoded this must be changed?

for t in $track_script_val; do
config_foreach print_track_script_indent vrrp_script "$t" "$INDENT_2"
done
printf '%b}\n' "${INDENT_1}" >> "$KEEPALIVED_CONF"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use config_section_close there?

printf '%b%s {\n' "${INDENT_1}" "$opt" >> "$KEEPALIVED_CONF"
for t in $optval; do
config_foreach print_track_script_indent track_script "$t" "$INDENT_2"
config_foreach print_track_script_indent vrrp_script "$t" "$INDENT_2"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you do, but can you please explain why this is needed?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently theres no support for track_script section, only for vrrp_script.
So inside print_track_script_indent what config_get does (from my understanding) is tries to fetch track_script section, which doesnt exist. But vrrp_script does.

I am not sure if this is how it was supposed to be done so correct me if I'm wrong but I tested this and it works.
In the future once track_script section is supported then we can add support for both here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the old option is still valid, we need to extend it to include the new option too. What would a correct keepalived configuration look like if both the track_script and the vrrp_script options is set in the uci.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old option doesn't exist in the code. However its still present in the keepalived.config as of now.

This is the sample configuration

config vrrp_instance 'vrrp_instance_10'
        option name 'vrrp_instance_10'
        option state 'MASTER'
        option interface 'br-i10'
        list virtual_ipaddress 'floatingIPaddr_10'
        option virtual_router_id '100'
        option priority '100'
        option advert_int '1'
        option preempt_delay '0'
        option auth_type 'PASS'
        option auth_pass 'pass'
        #list track_script 'sla_check'

config ipaddress 'floatingIPaddr_10'
        option name 'floatingIPaddr_10'
        option address '192.168.10.1/24'
        option device 'br-i10'
        option scope 'global'

config vrrp_sync_group 'vrrp_sync_group'
        option name 'sync_group'
        list group 'vrrp_instance_10'
        list track_script 'sla_check'

config vrrp_script 'sla_check'
        option name 'sla_check'
        option timeout '1'
        option interval '1'
        option fall '2'
        option rise '2'
        #option weight '50'
        option script '/etc/scripts/vrrp_sla_check.sh'

config track_script 'script1'
        option name     "track_script1"
        option value "/etc/scripts/vrrp_sla_check.sh"
        option weight   "1"

The above config generates this keepalived.conf file

! Configuration file for keepalived (autogenerated via init script)
! Written Tue Mar 31 09:39:16 2026

global_defs {
}

static_ipaddress {
}

static_routes {
}

interface_up_down_delays {
}

vrrp_script sla_check {
        script /etc/scripts/vrrp_sla_check.sh
        interval 1
        fall 2
        rise 2
        timeout 1
}

vrrp_sync_group sync_group {
        group {
                vrrp_instance_10
        }
        track_script {
                sla_check
        }
        notify_backup "/bin/busybox env -i ACTION=NOTIFY_BACKUP TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify_master "/bin/busybox env -i ACTION=NOTIFY_MASTER TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify_fault "/bin/busybox env -i ACTION=NOTIFY_FAULT TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify "/bin/busybox env -i ACTION=NOTIFY TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
}

vrrp_instance vrrp_instance_10 {
        authentication {
                auth_type PASS
                auth_pass pass
        }
        state MASTER
        interface br-i10
        virtual_router_id 100
        priority 100
        advert_int 1
        preempt_delay 0
        notify_backup "/bin/busybox env -i ACTION=NOTIFY_BACKUP TYPE=INSTANCE NAME=vrrp_instance_10 /sbin/hotplug-call keepalived"
        notify_master "/bin/busybox env -i ACTION=NOTIFY_MASTER TYPE=INSTANCE NAME=vrrp_instance_10 /sbin/hotplug-call keepalived"
        notify_fault "/bin/busybox env -i ACTION=NOTIFY_FAULT TYPE=INSTANCE NAME=vrrp_instance_10 /sbin/hotplug-call keepalived"
        notify_stop "/bin/busybox env -i ACTION=NOTIFY_STOP TYPE=INSTANCE NAME=vrrp_instance_10 /sbin/hotplug-call keepalived"
        virtual_ipaddress {
                192.168.10.1/24 dev br-i10 label br-i10:vip scope global
        }
}

As you can see no section for track_script was generated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then I do not understand the uci why do i have to specifies the path to the script /ec/scripts/vrr_sla_check.sh twice in vrrp_script and track_script section. In the generate keepalived.conf the track_script references only to thevrrp_script section. In your case sla_check.
I’ll have to take a closer look at that when I get the more time.

config vrrp_script 'sla_check'
        option name 'sla_check'
        option timeout '1'
        option interval '1'
        option fall '2'
        option rise '2'
        #option weight '50'
        option script '/etc/scripts/vrrp_sla_check.sh'

config track_script 'script1'
        option name     "track_script1"
        option value "/etc/scripts/vrrp_sla_check.sh"
        option weight   "1"
vrrp_script sla_check {
        script /etc/scripts/vrrp_sla_check.sh
        interval 1
        fall 2
        rise 2
        timeout 1
}

vrrp_sync_group sync_group {
        group {
                vrrp_instance_10
        }
        track_script {
                sla_check
        }
        notify_backup "/bin/busybox env -i ACTION=NOTIFY_BACKUP TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify_master "/bin/busybox env -i ACTION=NOTIFY_MASTER TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify_fault "/bin/busybox env -i ACTION=NOTIFY_FAULT TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
        notify "/bin/busybox env -i ACTION=NOTIFY TYPE=GROUP NAME=sync_group /sbin/hotplug-call keepalived"
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, we dont need the section track_script. On reviewing the docs of keepalived, a seprate track_script section doesnt exist. My guess is that it was removed and the config file was not updated, same with the init file.

I think we should update keepalived.config accordingly

Removed an if statement that got pushed by mistake

Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
Signed-off-by: Rishabh <rishabhshah2005@gmail.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the OpenWrt keepalived package integration to support generating track_script { ... } inside vrrp_sync_group blocks (using vrrp_script references), adjusts how track_script entries are rendered, and refreshes the example UCI config accordingly.

Changes:

  • Add track_script block generation to vrrp_sync_group.
  • Switch track_script lookups in instances/groups to use vrrp_script sections and extend vrrp_script emission with timeout.
  • Update example config and bump PKG_RELEASE.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
net/keepalived/Makefile Bumps package release to publish the integration changes.
net/keepalived/files/keepalived.init Adds sync-group track_script generation and changes track-script rendering / lookup logic.
net/keepalived/files/keepalived.config Updates example configuration/comments for the new track_script/vrrp_script behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 216 to 220
local name value weight direction
config_get name "$section" name
[ "$name" != "$curr_track_elem" ] && return 0
[ -z "$name" ] && return 0

config_get value "$section" value
config_get weight "$section" weight
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

print_track_script_indent() currently has a shell syntax error (if [ -z "$name" ] && return 0) and it no longer filters by curr_track_elem. As a result, every config_foreach ... vrrp_script invocation will print all vrrp_script sections (and, in the current loops, likely print them repeatedly), instead of only the script referenced by the list entry. Replace the invalid if with a normal test, and restore a name != curr_track_elem guard (while still printing the referenced script name).

Copilot uses AI. Check for mistakes.
Comment on lines +79 to +83
# list group "VI_1"
# list group "VI_2"
# Note that priority will not work with vrrp_script
# when in sync group
# list track_script "vrrp_script1"
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sync-group example now shows list track_script "vrrp_script1", but the config template still includes (elsewhere) the old config track_script sections and vrrp_instance examples referencing track_script1/track_script2. Since the init script now looks up track_script entries via vrrp_script sections, please update the remaining examples to reference vrrp_script names (and remove/adjust the config track_script examples) to avoid misleading users and potential breakage on upgrade.

Copilot uses AI. Check for mistakes.
# option rise "3"
# valid values for direction reverse|noreverse -- reverse flips weight change
# option direction "reverse"
# option timeout "5"
Copy link

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the vrrp_script example, the newly added timeout line has inconsistent spacing (#\toption timeout "5"). For readability and consistency with the rest of the template, align it to the same option <key>\t\t"<val>" format used by the surrounding options.

Suggested change
# option timeout "5"
# option timeout "5"

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants