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

Online image change: handling of the standby subcluster #127

Merged
merged 9 commits into from
Dec 16, 2021

Conversation

spilchen
Copy link
Collaborator

This is another PR for online-upgrade. It will handle creation and removal of the standby subcluster during the online-image change process.

  • new state was added to the vapi.Subcluster for this. Originally, I was planning to keep most of this in the SubclusterHandle struct, but we already pass around vapi.Subcluster so it made it easier to have it their
  • new status conditions for offline and online image change. These are intended to be used by the operator to know what image change to continue with once an image change has started
  • filled out more of the logic in onlineimagechange_reconciler.go. It will scale-out a new standby subcluster for each primary, then scale them down when we are finishing the image change.
  • moved more logic into imagechange.go that is common between online and offline image change
  • restart logic was changed to allow option to restart read-only nodes. When restarting for online, we will skip the read-only modes. Offline restarts everything.

@spilchen spilchen requested a review from roypaulin December 16, 2021 15:18
@spilchen spilchen self-assigned this Dec 16, 2021

// +kubebuilder:validation:Optional
// +operator-sdk:csv:customresourcedefinitions:type=spec,xDescriptors="urn:alm:descriptor:com.tectonic.ui:hidden"
// If a standby, this is the name of the primary subcluster it is a standby
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a missing word on this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about this?

// If this is a standby, this is the name of the primary subcluster the
// subcluster was created for.  This is state internally managed for
// an online image change. 

Copy link
Collaborator

@roypaulin roypaulin Dec 16, 2021

Choose a reason for hiding this comment

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

Yeah I think it is better. Maybe because my english is no so good but I feel like having 2 consecutive occurrences of subscluster is repetitive. What do you think about:


// If this is a standby subcluster, this is the name of the primary subcluster 
// it was created for.  This is state internally managed for
// an online image change. 

Annotations: makeAnnotationsForObject(vdb),
},
Spec: appsv1.StatefulSetSpec{
Selector: &metav1.LabelSelector{
MatchLabels: makeSvcSelectorLabels(vdb, scHandle),
MatchLabels: makeSvcSelectorLabels(vdb, sc),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is sticking to sc instead of scHandle only due to the fact that vapi.Subcluster is already passed around?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, so this undoes some of the work that I did earlier. Overall, I think it makes it simpler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok got it.

@roypaulin
Copy link
Collaborator

looks good. let's wait for the checks.

@roypaulin
Copy link
Collaborator

looks good. Thanks.

@spilchen spilchen merged commit c761ce2 into vertica:online-upgrade Dec 16, 2021
@spilchen spilchen deleted the standby-handling branch December 16, 2021 21:10
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.

2 participants