-
Notifications
You must be signed in to change notification settings - Fork 24
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
set DisableNonReplicatableQueries before sandboxing subcluster during online upgrade #853
set DisableNonReplicatableQueries before sandboxing subcluster during online upgrade #853
Conversation
3a0d601
to
c89ff2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. For testing your code, you could add step 36 in
e2e-leg-9/new-online-upgrade-sanity to verify non-replicable queries cannot be made in old main cluster, and add step 47 to verify we can make non-replicable queries in new main cluster.
You don't need to modify the added test step(36). It looks good. Once you fixed the conflicts and passed the test, I will approve it. |
544e1b8
to
7411a28
Compare
pkg/controllers/vdb/upgrade.go
Outdated
return ctrl.Result{Requeue: true}, nil | ||
} | ||
|
||
cmd := []string{"-tAc", "SELECT count(*) FROM node_subscriptions WHERE subscription_state not in ('ACTIVE', 'INACTIVE');"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the subscriptions be "INACTIVE"? Why not using "subscription_state != 'ACTIVE'"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
running a test, yes. subscriptions can be inactive and stay that way. for our use case we don't really care, this check is enough to make sure that sandbox can be run successfully
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is: if subscriptions on the nodes of a subcluster are inactive, the subcluster doesn't have coverage to do the sandbox. What do I miss?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not sure. all i know is that running rebalance_shards didn't change the subscription_state and sandboxing went fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subscription_state probably is "PENDING" after add_node(before rebalance_shards) so this check fixed the issue. But the safer way is to verify all subscriptions are "ACTIVE".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge the main into your branch to get the latest changes.
Manager UpgradeManager | ||
Dispatcher vadmin.Dispatcher | ||
sandboxName string // name of the sandbox created for replica group B | ||
originalConfigParamDisableNonReplicatableQueriesValue string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we going to set back the original value when upgrade is done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? this is an undocumented parameter that, eg, prevents reip for now and is purely needed for upgrade. once we've addressed the issues on the server side i'd be open to setting it, especially if we document it, but for now i think we should leave it cleared in the new cluster
cef7c7a
to
05ef7fd
Compare
05ef7fd
to
ef15842
Compare
288b664
to
dba7c3f
Compare
ef5a218
to
2f15656
Compare
…ries session scoped
mostly based on the work in #831 plus fixing merge conflicts, etc