Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-404512: Add feature flag to the new clustering interface #6217

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

Vincent-lau
Copy link
Contributor

The new clustering interface uses a constructor Extended address when a cluster hots is trying to join a cluster. This causes problems during upgrades as a newly upgraded host might send out the new address format to old hosts, which do not understand this format, causing the new hosts not able to join.

The fix would be to use the cluster_health feature flag/pool restrictions to gate the use of the new clustering interface. This makes sure that the new interface would only be used if all of the hosts understand this new interface, i.e. have this feature enabled. The cluster_health feature is controlled by v6d and is pool-wide, therefore the new interface would only be enabled if all v6ds are updated to the correct level, which also implies that the accompanying xapi are updated to the correct level.

@Vincent-lau
Copy link
Contributor Author

Vincent-lau commented Jan 9, 2025

This is likely end up being a high priority push, so once this is reviewed and merged into master, I will find an appropriate tag onto which we can base this commit (probably 24.39.0), and then make a release out of it.

@last-genius
Copy link
Contributor

This is likely end up being a high priority push, so once this is reviewed and merged into master, I will find an appropriate tag onto which we can base this commit (probably 24.39.0), and then make a release out of it.

Is this how this works? Would we then have to rebase the current master on top of this commit+24.39.0 since otherwise history would not make sense?
There's only a few fairly small commits you'd grab by taking this fix+current master as the tag to work with

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

Concise fix

@Vincent-lau Vincent-lau force-pushed the private/shul2/ch-flag branch from 74886de to b6a8dcc Compare January 10, 2025 11:37
@Vincent-lau
Copy link
Contributor Author

This PR would need to be merged alongside with a v6 PR, which introduces the new cluster_address feature

@robhoes
Copy link
Member

robhoes commented Jan 10, 2025

Do we have a problem when updating a pool from an older version that already uses the extended address type? I think it tend reverts to non-extended temporarily during the update... is that going to go wrong?

@Vincent-lau
Copy link
Contributor Author

Vincent-lau commented Jan 10, 2025

Do we have a problem when updating a pool from an older version that already uses the extended address type? I think it tend reverts to non-extended temporarily during the update... is that going to go wrong?

Yes good point. I think this should be fine, as the clustering interface is defined and known to all hosts that is using the extended address, and even if one of them revert to the old address type, other hosts that have not applied this patch would still be able to understand that.

Alternative would be to just keep using the cluster_health feature flag, instead of the new cluster_address feature flag, which will make sure those who know this interface will keep using it

Comment on lines 68 to 76
if Xapi_cluster_helpers.cluster_address_enabled ~__context then
Extended
{
ip= Ipaddr.of_string_exn (ipstr_of_address ip_addr)
; hostuuid
; hostname
}
else
IPv4 (ipstr_of_address ip_addr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this piece of code also used below, can we add a util in Xapi_cluster_helpers for this?

The new clustering interface uses a constructor `Extended` address when
a cluster hots is trying to join a cluster. This causes problems during
upgrades as a newly upgraded host might send out the new address format
to old hosts, which do not understand this format, causing the new hosts
not able to join.

The fix would be to use a new cluster_address feature flag/pool
restrictions to control the use of the new clustering interface. This makes
sure that the new interface would only be used if all of the hosts
understand this new interface, i.e. have this feature enabled. The
cluster_address feature is controlled by v6d and is pool-wide, therefore
the new interface would only be enabled if all v6ds are updated to the
correct level, which also implies that the accompanying xapi are updated
to the correct level.

Signed-off-by: Vincent Liu <[email protected]>
@Vincent-lau Vincent-lau force-pushed the private/shul2/ch-flag branch from b6a8dcc to 02ca33e Compare January 13, 2025 13:03
@Vincent-lau Vincent-lau added this pull request to the merge queue Jan 13, 2025
Merged via the queue into xapi-project:master with commit f2f6d20 Jan 13, 2025
15 checks passed
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.

5 participants