Update upgrade reconciler to handle subclusters in sandbox #772

roypaulin · 2024-04-16T00:03:49Z

This change will modify the upgrade reconcilers, both offline and online, to function in either the main cluster or a sandbox. Our immediate plan is to use the offline upgrade reconciler within the sandbox controller, although this will be done in a follow-on task.

…ndbox

spilchen · 2024-04-16T12:34:42Z

pkg/iter/sc_finder.go

+	// we can skip the filtering if all of the sts we are
+	// looking for are not for a subcluster in vdb
+	if flags&FindInVdb != 0 || flags&FindExisting != 0 {
+		sts = m.filterStatefulsetsBySandbox(sts, sandbox)


Can we add the filtering in m.buildObjList. If we search for the vertica.com/sandbox label, it should be able to find the sts we are looking for. Have we decided if the pods will have a sandbox label either? We may be able to filter pods in the same manner.

We can add that label to both for filtering, but shouldn't we only trust the sandbox status in vdb? That's what tell us which sts/pod belongs to a sandbox. I guess we can also use a label but in that case we need to make sure we correctly set/unset it.

It should be setup correctly. We have full control over it, so it would a programming error if not set.

Ok then let's set the label in both sts and pods.

spilchen · 2024-04-16T14:00:54Z

pkg/controllers/vdb/upgrade.go

 	isAllowedForUpgradePolicyFunc func(vdb *vapi.VerticaDB) bool) *UpgradeManager {
 	return &UpgradeManager{
 		VRec:                          vdbrecon,
 		Vdb:                           vdb,
 		Log:                           log,
 		Finder:                        iter.MakeSubclusterFinder(vdbrecon.Client, vdb),
 		StatusCondition:               statusCondition,
+		SandboxName:                   sandbox,


I don't think setting sandbox for the entire upgrade manager makes sense. For replicated upgrade, it will span both the main cluster and a sandbox. So, it doesn't really make sense. I think each function in UpgradeManager that needs the sandbox should accept it as input.

spilchen · 2024-04-16T14:05:16Z

pkg/controllers/vdb/obj_reconciler.go

@@ -233,7 +233,7 @@ func (o *ObjReconciler) checkForDeletedSubcluster(ctx context.Context) (ctrl.Res
 	finder := iter.MakeSubclusterFinder(o.VRec.Client, o.Vdb)

 	// Find any statefulsets that need to be deleted
-	stss, err := finder.FindStatefulSets(ctx, iter.FindNotInVdb)
+	stss, err := finder.FindStatefulSets(ctx, iter.FindNotInVdb, vapi.MainCluster)


Rather than hard coding the main cluster, could we use the sandbox found in the podfacts?

spilchen · 2024-04-16T14:06:12Z

pkg/controllers/vdb/offlineupgrade_reconciler.go

@@ -228,7 +230,7 @@ func (o *OfflineUpgradeReconciler) checkNMADeploymentChange(ctx context.Context)
 		return ctrl.Result{}, nil
 	}

-	stss, err := o.Finder.FindStatefulSets(ctx, iter.FindExisting)
+	stss, err := o.Finder.FindStatefulSets(ctx, iter.FindExisting, o.SandboxName)


General comment, could we use the sandbox from the podfacts if its available in the reconciler?

spilchen · 2024-04-17T11:55:29Z

pkg/builder/labels_annotations.go

+	labels := makeLabelsForObject(vdb, sc, true)
+	sandbox := vdb.GetSubclusterSandboxName(sc.Name)
+	if sandbox != vapi.MainCluster {
+		labels[vmeta.SandboxNameLabel] = sandbox


We should update the comment for vmeta.SandboxNameLabel (in pkg/meta/labels.go) to mention that we add it for pods now.

spilchen · 2024-04-17T11:57:19Z

pkg/controllers/vdb/podfacts.go

@@ -1129,6 +1129,17 @@ func (p *PodFacts) findExpectedNodeNames() []string {
 	return expectedNodeNames
 }

+// getSandboxName returns the name of the sandbox, or empty string
+// for main cluster, the pods belong to
+func (p *PodFacts) getSandboxName() string {


As prep for moving this struct to its own package, we probably want to export it.

Suggested change

func (p *PodFacts) getSandboxName() string {

func (p *PodFacts) GetSandboxName() string {

pkg/controllers/vdb/upgrade.go

spilchen · 2024-04-17T12:09:27Z

pkg/iter/sc_finder.go

@@ -80,7 +80,7 @@ func (m *SubclusterFinder) FindStatefulSets(ctx context.Context, flags FindFlags
 // FindServices returns service objects that are in use for subclusters
 func (m *SubclusterFinder) FindServices(ctx context.Context, flags FindFlags) (*corev1.ServiceList, error) {
 	svcs := &corev1.ServiceList{}
-	if err := m.buildObjList(ctx, svcs, flags); err != nil {
+	if err := m.buildObjList(ctx, svcs, flags, vapi.MainCluster); err != nil {


This is correct, just because we know how FindServices is called, but to be consistent with the other exported APIs, we should take the sandbox in as input parm to FindServices. That will keep this package general purpose.

spilchen · 2024-04-17T12:12:00Z

pkg/iter/sc_finder.go

@@ -125,7 +120,7 @@ func (m *SubclusterFinder) FindSubclusters(ctx context.Context, flags FindFlags,
 	// a sandboxed subcluster can only be one of those in the vdb so we skip this part if
 	// we are looking for sandboxed subclusters
 	if isMainCluster && (flags&FindNotInVdb != 0 || flags&FindExisting != 0) {
-		missingSts, err := m.FindStatefulSets(ctx, flags & ^FindInVdb)
+		missingSts, err := m.FindStatefulSets(ctx, flags & ^FindInVdb, vapi.MainCluster)


Does this still make sense to pass in main cluster here? Any harm in removing the isMainCluster, getting in here for a sandbox and passing the sandbox parm here. It would keep this logic simple. No need for the comment at line 120-121.

This was to avoid calling the client to fetch sts when we don't need, but I see what you mean, now that we have moved the filter in buildObjList let's just pass the sandbox name.

spilchen · 2024-04-17T12:14:27Z

pkg/iter/sc_finder.go

+	if sandbox == vapi.MainCluster {
+		return l[vmeta.SandboxNameLabel] != vapi.MainCluster
+	}
+	return l[vmeta.SandboxNameLabel] != sandbox


Couldn't you just simply it this way?

Suggested change

if sandbox == vapi.MainCluster {

return l[vmeta.SandboxNameLabel] != vapi.MainCluster

}

return l[vmeta.SandboxNameLabel] != sandbox

return l[vmeta.SandboxNameLabel] != sandbox

spilchen · 2024-04-17T12:16:49Z

pkg/controllers/vdb/replicatedupgrade_reconciler.go

+	sandbox := r.PFacts.getSandboxName()
+	if ok, err := r.Manager.IsUpgradeNeeded(ctx, sandbox); !ok || err != nil {


When you merge up r.PFacts is a map now (sandbox name to podfacts). This only needs to look at the main cluster, so you probably can just pass in vapi.MainCluster here.

Suggested change

sandbox := r.PFacts.getSandboxName()

if ok, err := r.Manager.IsUpgradeNeeded(ctx, sandbox); !ok || err != nil {

if ok, err := r.Manager.IsUpgradeNeeded(ctx, vapi.MainCluster); !ok || err != nil {

…ndbox

spilchen

Everything looks good. Thanks for doing the revisions. I noticed in the last run leg 9 failed related to replicated upgrade. I didn't look at it in too much detail. So, I'm not sure if its a flaky test or not. I'm approving assuming it's a flaky test or requires a small change to fix.

roypaulin · 2024-04-17T14:14:05Z

Everything looks good. Thanks for doing the revisions. I noticed in the last run leg 9 failed related to replicated upgrade. I didn't look at it in too much detail. So, I'm not sure if its a flaky test or not. I'm approving assuming it's a flaky test or requires a small change to fix.

I think it is a flaky test, I got that yesterday and rerun the test. At wait-for-secondaries only 2/3 nodes are up. If it persists we would need to take a closer look.

roypaulin · 2024-04-18T06:14:42Z

pkg/builder/labels_annotations.go

@@ -88,6 +88,12 @@ func makeLabelsForObject(vdb *vapi.VerticaDB, sc *vapi.Subcluster, forPod bool)

 // MakeLabelsForPodObject constructs the labels that are common for all pods
 func MakeLabelsForPodObject(vdb *vapi.VerticaDB, sc *vapi.Subcluster) map[string]string {
+	return makeLabelsForObject(vdb, sc, true)


I think the test was failing because when the replicaB is sandboxed, the sts podTemplateSpec is updated(sandbox name label added) which causes a pod replacement with a rollingUpdateStrategy. That's why *-pri1-sb-2 is restarted(and will never be up again as restart reconciler is not called at this point) and we get upNodeCount mismatch expect 3 actual 2. I don't think we really want to rolling update those pods, do we?
So if we want to add sandbox label to the pods, maybe we should do so after the sandbox reconciler. What do you think?

Yeah, makes sense. Since the pods have to be restarted by the sandbox controller, it seems like it should apply the label then.

This change will modify the upgrade reconcilers, both offline and online, to function in either the main cluster or a sandbox. Our immediate plan is to use the offline upgrade reconciler within the sandbox controller, although this will be done in a follow-on task.

roypaulin added 2 commits April 15, 2024 22:48

Update upgrade reconciler

f414424

Merge remote-tracking branch 'origin/vnext' into roypaulin/upgrade-sa…

5b71d1a

…ndbox

roypaulin requested a review from spilchen as a code owner April 16, 2024 00:03

spilchen reviewed Apr 16, 2024

View reviewed changes

roypaulin added 2 commits April 16, 2024 23:14

Address comments

5b438ef

Remove trailing line

2c0220d

roypaulin requested a review from spilchen April 16, 2024 21:22

roypaulin self-assigned this Apr 16, 2024

spilchen reviewed Apr 17, 2024

View reviewed changes

roypaulin added 2 commits April 17, 2024 14:31

Merge remote-tracking branch 'origin/vnext' into roypaulin/upgrade-sa…

ac14b13

…ndbox

Address review comments

05a527a

roypaulin requested a review from spilchen April 17, 2024 13:28

spilchen approved these changes Apr 17, 2024

View reviewed changes

Fix e2e test

bfd2bf2

roypaulin commented Apr 18, 2024

View reviewed changes

roypaulin merged commit 3773a48 into vnext Apr 19, 2024
31 checks passed

roypaulin deleted the roypaulin/upgrade-sandbox branch April 19, 2024 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update upgrade reconciler to handle subclusters in sandbox #772

Update upgrade reconciler to handle subclusters in sandbox #772

roypaulin commented Apr 16, 2024 •

edited by spilchen

Loading

spilchen Apr 16, 2024

roypaulin Apr 16, 2024

spilchen Apr 16, 2024

roypaulin Apr 16, 2024

spilchen Apr 16, 2024

spilchen Apr 16, 2024

spilchen Apr 16, 2024

spilchen Apr 17, 2024

spilchen Apr 17, 2024

spilchen Apr 17, 2024

spilchen Apr 17, 2024

roypaulin Apr 17, 2024

spilchen Apr 17, 2024

spilchen Apr 17, 2024

spilchen left a comment

roypaulin commented Apr 17, 2024 •

edited

Loading

roypaulin Apr 18, 2024 •

edited

Loading

spilchen Apr 19, 2024

	func (p *PodFacts) getSandboxName() string {
	func (p *PodFacts) GetSandboxName() string {

		sandbox := r.PFacts.getSandboxName()
		if ok, err := r.Manager.IsUpgradeNeeded(ctx, sandbox); !ok \|\| err != nil {

Update upgrade reconciler to handle subclusters in sandbox #772

Update upgrade reconciler to handle subclusters in sandbox #772

Conversation

roypaulin commented Apr 16, 2024 • edited by spilchen Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spilchen left a comment

Choose a reason for hiding this comment

roypaulin commented Apr 17, 2024 • edited Loading

roypaulin Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roypaulin commented Apr 16, 2024 •

edited by spilchen

Loading

roypaulin commented Apr 17, 2024 •

edited

Loading

roypaulin Apr 18, 2024 •

edited

Loading