webhook rules for sandbox shutdown #971

LiboYu2 · 2024-10-28T16:44:51Z

Here are the rules that are implemented by this change.

When creating a vdb, all subclusters and sandboxes(if any) must not have shutdown set to true.
If a sandbox has shutdown(spec) true and any of its subclusters has the annotation "vertica.com/shutdown-driven-by-sandbox" set to true in the old vdb, we should prevent the user from updating that subcluster's shutdown field.
You cannot unsandbox a subcluster with shutdown true(spec and/or status) or part of a sandbox with shutdown true
You cannot change spec.sandboxes[].image if the sandbox has shutdown true or has any subcluster with shutdown true(spec and/or status)
You cannot terminate a sandbox if it has has shutdown true. terminate means removing the sandbox in spec.sandboxes[] and its subclusters in spec.subclusters[] at the same time.

roypaulin · 2024-10-30T11:51:09Z

api/v1/verticadb_webhook.go

+func (v *VerticaDB) checkSboxForShutdownInSpecAndStatus(sandbox *Sandbox, subclusterMap map[string]*Subcluster, subclusterIndexMap map[string]int, errMsgs []string) []string {
+	errMsgs = v.checkSboxForShutdownInSpec(sandbox, subclusterMap, errMsgs)
+	for _, subcluster := range sandbox.Subclusters {
+		if v.Status.Subclusters[subclusterIndexMap[subcluster.Name]].Shutdown {
+			errMsgs = append(errMsgs, fmt.Sprintf("Shutdown status of subcluster %q is true", subcluster.Name))
+		}
+	}
+	return errMsgs
+}
+
+func (v *VerticaDB) checkSboxForShutdownInSpec(sandbox *Sandbox, subclusterMap map[string]*Subcluster, errMsgs []string) []string {
+	if sandbox.Shutdown {
+		errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of sandbox %q is set to true", sandbox.Name))
+	}
+	for _, subcluster := range sandbox.Subclusters {
+		if subclusterMap[subcluster.Name].Shutdown {
+			errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of subcluster %q is set to true", subcluster.Name))
+		}
+	}
+	return errMsgs
+}


This will create too many msgs. Let's have a single loop over sandbox.Subclusters and for each subcluster find the shutdown value in both spec and status(make sure the subcluster exists in status). If any of them is true return a generate a generic msg like subcluster %q is marked for shut down or has already been shut down

"make sure the subcluster exists in status"
I got your point. When a user tries to change the sandbox image, the subcluster is already shutdown at this time. This is different from my original understanding. I will make a change.

This is from the JIRA:
"You cannot change spec.sandboxes[].image if the sandbox has shutdown true or has any subcluster with shutdown true(spec and/or status)"

Could you remove the "or" from within the brackets? Thanks.

Why? I mean what I said. If you prefer here is what I mean You cannot change spec.sandboxes[].image if the sandbox has shutdown true or has any subcluster with shutdown true(spec or status)

Got it. It is different from your original comment: "for each subcluster find the shutdown value in both spec and status(make sure the subcluster exists in status)."

roypaulin · 2024-10-30T12:10:41Z

api/v1/verticadb_webhook.go

@@ -1701,6 +1760,162 @@ func (v *VerticaDB) checkSandboxPrimary(allErrs field.ErrorList, oldObj *Vertica
 	return allErrs
 }

+// validateUnsandboxShutdownConditions ensures we will not unsandbox a subcluster that has shutdown set to true, or a subcluster
+// in a sandbox where the shutdown field for the sandbox is set to true
+func (v *VerticaDB) validateUnsandboxShutdownConditions(old runtime.Object, allErrs field.ErrorList) field.ErrorList {


Split the content of this function into smaller functions to make it more readable. Unsandboxing means 2 things: 1. we removed one or multiple subclusters from sandbox or we remove one or multiple sandboxes from spec.sandboxes.
In both cases we need to identify subclusters to be unsandboxed and for each for them, check if the subcluster shutdown(spec/status) is true or if it is/was part of a sandbox that has shutdown true.

"check if the subcluster shutdown(spec/status)"
for the condition shutdown(spec/status), is it an OR or AND? Currently I implemented it as an OR.

It is an OR.

We need to add one more limitation: we cannot add one or multiple subclusters to a sandbox if the sandbox is shutdown or partially shutdown. I didn't test it, but I think we cannot restart a sandboxed subcluster if that sandbox changed its group members.

The proposed new rule can be found in another webhook JIRA VER-97254

roypaulin · 2024-10-30T12:15:42Z

api/v1/verticadb_webhook.go

+	allErrs = v.hasNoShutdownSubclusters(allErrs)
+	allErrs = v.hasNoShutdownSandboxes(allErrs)


Let's change the 1st rule to this: when a subcluster or sandbox is added to the vdb for the 1st time (not in status yet) they all must have shutdown field set to false.

I believe the original 1st rule is still a valid one. The two cover different scenarios. Should we add what you proposed as a new rule?

Why don't we allow shutdown in db creation?

No, this new rule covers the original first rule. When you create a vdb, the subclusters and/or sandboxes in the spec will not be in the status.

roypaulin · 2024-10-30T16:51:00Z

api/v1/verticadb_webhook.go

+		if newSandbox.Shutdown {
+			for _, subclusterName := range oldSandbox.Subclusters {
+				oldSubcluster := oldSubclusterMap[subclusterName.Name]
+				if drivenby, ok := oldSubcluster.Annotations["vertica.com/shutdown-driven-by-sandbox"]; ok && drivenby == trueString {


There is already a constant in annotations.go. A function to get the annotation's value too.

roypaulin · 2024-10-30T16:53:28Z

api/v1/verticadb_webhook_test.go

+			Ω(newVdb.validateAnnotatedSubclustersInShutdownSandbox(oldVdb, field.ErrorList{})).Should(HaveLen(1))
+			newVdb.Spec.Subclusters[1].Shutdown = true
+			Ω(newVdb.validateAnnotatedSubclustersInShutdownSandbox(oldVdb, field.ErrorList{})).Should(HaveLen(0))
+


Suggested change

roypaulin · 2024-10-30T16:58:18Z

api/v1/verticadb_webhook_test.go

+			newVdb.Spec.Subclusters[1].Shutdown = false
+			Ω(newVdb.validateAnnotatedSubclustersInShutdownSandbox(oldVdb, field.ErrorList{})).Should(HaveLen(1))
+			newVdb.Spec.Subclusters[1].Shutdown = true
+			Ω(newVdb.validateAnnotatedSubclustersInShutdownSandbox(oldVdb, field.ErrorList{})).Should(HaveLen(0))


You also need the following cases:

change sc1 shutdown and check there is no error as it is not part of the sandbox

set sandbox.shutdown to false, rerun the above cases and check there is no error.

new cases added

roypaulin · 2024-10-30T16:59:34Z

api/v1/verticadb_webhook.go

@@ -1701,6 +1760,162 @@ func (v *VerticaDB) checkSandboxPrimary(allErrs field.ErrorList, oldObj *Vertica
 	return allErrs
 }

+// validateUnsandboxShutdownConditions ensures we will not unsandbox a subcluster that has shutdown set to true, or a subcluster
+// in a sandbox where the shutdown field for the sandbox is set to true
+func (v *VerticaDB) validateUnsandboxShutdownConditions(old runtime.Object, allErrs field.ErrorList) field.ErrorList {


It is an OR.

roypaulin · 2024-10-30T17:02:47Z

api/v1/verticadb_webhook.go

+func (v *VerticaDB) checkSboxForShutdownInSpecAndStatus(sandbox *Sandbox, subclusterMap map[string]*Subcluster, subclusterIndexMap map[string]int, errMsgs []string) []string {
+	errMsgs = v.checkSboxForShutdownInSpec(sandbox, subclusterMap, errMsgs)
+	for _, subcluster := range sandbox.Subclusters {
+		if v.Status.Subclusters[subclusterIndexMap[subcluster.Name]].Shutdown {
+			errMsgs = append(errMsgs, fmt.Sprintf("Shutdown status of subcluster %q is true", subcluster.Name))
+		}
+	}
+	return errMsgs
+}
+
+func (v *VerticaDB) checkSboxForShutdownInSpec(sandbox *Sandbox, subclusterMap map[string]*Subcluster, errMsgs []string) []string {
+	if sandbox.Shutdown {
+		errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of sandbox %q is set to true", sandbox.Name))
+	}
+	for _, subcluster := range sandbox.Subclusters {
+		if subclusterMap[subcluster.Name].Shutdown {
+			errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of subcluster %q is set to true", subcluster.Name))
+		}
+	}
+	return errMsgs
+}


Why? I mean what I said. If you prefer here is what I mean You cannot change spec.sandboxes[].image if the sandbox has shutdown true or has any subcluster with shutdown true(spec or status)

roypaulin · 2024-10-30T17:08:20Z

api/v1/verticadb_webhook.go

+	allErrs = v.hasNoShutdownSubclusters(allErrs)
+	allErrs = v.hasNoShutdownSandboxes(allErrs)


No, this new rule covers the original first rule. When you create a vdb, the subclusters and/or sandboxes in the spec will not be in the status.

Co-authored-by: Roy Paulin <[email protected]>

roypaulin

We are getting close.

api/v1/verticadb_webhook.go

roypaulin · 2024-11-04T20:00:04Z

api/v1/verticadb_webhook.go

+// checkSboxForShutdown checks if sbox or its scluster has shutdown field set to true in spec/status
+func (v *VerticaDB) checkSboxForShutdown(newSandbox *Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) string {
+	if newSandbox.Shutdown {
+		return fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name)


Suggested change

return fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name)

errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name))

will make the change.

Yes, but it can happen that the sandbox has shutdown true and also all its subclusters have shutdown true. In that case we want to report all the error msgs so the user is aware.

roypaulin · 2024-11-04T20:00:43Z

api/v1/verticadb_webhook.go

+	for _, subcluster := range newSandbox.Subclusters {
+		i, foundInStatus := statusSClusterIndexMap[subcluster.Name]
+		if foundInStatus && (newSClusterMap[subcluster.Name].Shutdown || v.Status.Subclusters[i].Shutdown) {
+			return fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name)


Suggested change

return fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name)

errMsgs = append(errMsgs, fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name))

same comment as the above one.

roypaulin · 2024-11-04T20:00:56Z

api/v1/verticadb_webhook.go

+			return fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name)
+		}
+	}
+	return ""


Suggested change

return ""

return errMsgs

same comment as the above one. When it returns empty string, it means no error.

roypaulin · 2024-11-04T20:01:42Z

api/v1/verticadb_webhook.go

+}
+
+// checkSboxForShutdown checks if sbox or its scluster has shutdown field set to true in spec/status
+func (v *VerticaDB) checkSboxForShutdown(newSandbox *Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) string {


Suggested change

func (v *VerticaDB) checkSboxForShutdown(newSandbox *Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) string {

func (v *VerticaDB) checkSboxForShutdown(newSandbox *Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) []string {

errMsgs := []string{}

We still want to return all the error msgs. We just did not want to return one batch for spec and another for status.

Now I know what you want to achieve. I withdraw my previous comments and will make the change.

roypaulin · 2024-11-04T20:21:37Z

api/v1/verticadb_webhook.go

+			allErrs = append(allErrs, err)
+			continue
+		}
+		if oldSandbox.Shutdown {
+			p := field.NewPath("spec").Child("sandboxes")
+			err := field.Invalid(p,
+				oldSubcluster.Name,
+				fmt.Sprintf("cannot unsandbox subcluster %q in sandbox %q that has Shutdown field set to true",
+					oldSubcluster.Name, oldSandbox.Name))
+			allErrs = append(allErrs, err)
+			continue
+		}


Suggested change

allErrs = append(allErrs, err)

continue

}

if oldSandbox.Shutdown {

p := field.NewPath("spec").Child("sandboxes")

err := field.Invalid(p,

oldSubcluster.Name,

fmt.Sprintf("cannot unsandbox subcluster %q in sandbox %q that has Shutdown field set to true",

oldSubcluster.Name, oldSandbox.Name))

allErrs = append(allErrs, err)

continue

}

allErrs = append(allErrs, err)

}

if oldSandbox.Shutdown {

p := field.NewPath("spec").Child("sandboxes").Index(...)

err := field.Invalid(p.Child("shutdown"),

oldSandbox.Name,

fmt.Sprintf("cannot unsandbox subcluster %q in sandbox %q that has shutdown field set to true",

oldSubcluster.Name, oldSandbox.Name))

allErrs = append(allErrs, err)

}

roypaulin · 2024-11-04T20:27:48Z

api/v1/verticadb_webhook.go

+						err := field.Invalid(p.Index(index),
+							subclusterName.Name,


Suggested change

err := field.Invalid(p.Index(index),

subclusterName.Name,

err := field.Invalid(p.Index(index).Child("shutdown"),

newSubcluster.Shutdown,

roypaulin · 2024-11-04T20:30:28Z

api/v1/verticadb_webhook.go

+						p := field.NewPath("spec").Child("subclusters")
+						err := field.Invalid(p.Index(index),
+							subclusterName.Name,
+							fmt.Sprintf("cannot change Shutdown field for subcluster %q that has annotation \"vertica.com/shutdown-driven-by-sandbox\" set to true",


Suggested change

fmt.Sprintf("cannot change Shutdown field for subcluster %q that has annotation \"vertica.com/shutdown-driven-by-sandbox\" set to true",

fmt.Sprintf("cannot change shutdown field for subcluster %q whose shutdown is driven by sandbox %q",

roypaulin · 2024-11-04T20:32:13Z

api/v1/verticadb_webhook.go

+		newSBox := v.Spec.Sandboxes[i]
+		if _, foundInStatus := statusSBoxMap[newSBox.Name]; !foundInStatus && newSBox.Shutdown {
+			p := field.NewPath("spec").Child("sandboxes").Index(i)
+			err := field.Invalid(p,


Suggested change

err := field.Invalid(p,

err := field.Invalid(p.Child("shutdown"),

roypaulin · 2024-11-04T20:33:11Z

api/v1/verticadb_webhook.go

+		newSCluster := &v.Spec.Subclusters[i]
+		if _, foundInStatus := statusSClusterMap[newSCluster.Name]; !foundInStatus && newSCluster.Shutdown {
+			p := field.NewPath("spec").Child("subclusters").Index(i)
+			err := field.Invalid(p,


Suggested change

err := field.Invalid(p,

err := field.Invalid(p.child("shutdown"),

Co-authored-by: Roy Paulin <[email protected]>

…kubernetes into VER-97368-webhook-rules

roypaulin

This is the last round. After my comments are addressed, I will approve

roypaulin · 2024-11-05T20:17:04Z

api/v1/verticadb_webhook.go

 	if newSandbox.Shutdown {
-		return fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name)
+		return append(errMsgs, fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name))


Do not return.

roypaulin · 2024-11-05T20:17:23Z

api/v1/verticadb_webhook.go

 	}

 	for _, subcluster := range newSandbox.Subclusters {
 		i, foundInStatus := statusSClusterIndexMap[subcluster.Name]
 		if foundInStatus && (newSClusterMap[subcluster.Name].Shutdown || v.Status.Subclusters[i].Shutdown) {
-			return fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name)
+			return append(errMsgs, fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name))


Do not return.
Also to make the error msg shorter, we can first append the subcluster names and then add the msg. The err msg will look like: ......Subclusters sc1, sc2, sc3 are marked for shut down or have already been shut down.

roypaulin · 2024-11-05T20:34:34Z

api/v1/verticadb_webhook.go

+		}
+		// sandboxToBeRemoved and all its subclusters are not found in new spec
+		if sclustersToTerminate == numOfSclustersInSbox {
+			if oldSandboxMap[sandboxToBeRemoved].Shutdown {


This check should be at the beginning of the loop so you can skip to the next sandbox if the current sandbox has shutdown true.

This if statement compares the number of subclusters to be removed with the number of subclusters in the sandbox.
If they equal, that means the sandbox will be terminated.
There is another for loop on the top of this if statement. It is used to calculate "sclustersToTerminate" used in the if statement.

Again, if oldSandboxMap[sandboxToBeRemoved].Shutdown is false then there is no point in running all this code. Check oldSandboxMap[sandboxToBeRemoved].Shutdown at the beginning of the loop and if false skip and go to next sandbox. Something like:

for _, sandboxToBeRemoved := range sandboxesToBeRemoved { if !oldSandboxMap[sandboxToBeRemoved].Shutdown { continue }

That is a good point. I will check oldSandboxMap[sandboxToBeRemoved].Shutdown earlier. If it is false, let it skip the rest of the current loop.

roypaulin · 2024-11-05T20:37:02Z

api/v1/verticadb_webhook.go

+	allErrs = v.validateShutdownSandboxImage(old, allErrs)
+	allErrs = v.validateTerminatingSandboxes(old, allErrs)
+	allErrs = v.validateUnsandboxShutdownConditions(old, allErrs)
+	allErrs = v.validateAnnotatedSubclustersInShutdownSandbox(old, allErrs)
+	allErrs = v.validateNewSBoxOrSClusterShutdownUnset(allErrs)


Let's keep the same function structure and call these check... like checkShutdownSandboxImage

roypaulin · 2024-11-05T20:39:30Z

api/v1/verticadb_webhook.go

@@ -1701,6 +1728,199 @@ func (v *VerticaDB) checkSandboxPrimary(allErrs field.ErrorList, oldObj *Vertica
 	return allErrs
 }

+// ensures a new subcluster or sandbox to be added to a vdb has Shutdown field set to false


Suggested change

// ensures a new subcluster or sandbox to be added to a vdb has Shutdown field set to false

// validateNewSBoxOrSClusterShutdownUnset ensures a new subcluster or sandbox to be added to a vdb has Shutdown field set to false

Follow the same forma for other functions

cchen-vertica · 2024-11-06T13:27:54Z

api/v1/helpers.go

+	return statusSboxMap
+}
+
+// GenStatusSandboxMap() returns a map from status. The key is subcluster name and value is the subcluster pointer


Minor: update GenStatusSandboxMap to GenStatusSubclusterMap

cchen-vertica · 2024-11-06T13:32:25Z

api/v1/verticadb_webhook.go

+func (v *VerticaDB) findPersistSandboxes(oldObj *VerticaDB) []string {
+	oldSandboxes := oldObj.Spec.Sandboxes
+	newSandboxes := v.Spec.Sandboxes
+	oldSandboxMap := make(map[string]int)


Minor: this map should be a map[string]any or map[string]bool.

This needs to be addressed.

cchen-vertica · 2024-11-06T13:39:16Z

api/v1/verticadb_webhook.go

+	return allErrs
+}
+
+// findSclustersToUnsandboxreturn a map whose key is the pointer of a subcluster that is to be unsandboxed.


add space in front of "return"

cchen-vertica · 2024-11-06T13:47:06Z

api/v1/verticadb_webhook.go

+	newSubclusterMap := v.GenSubclusterMap()
+	sclusterSboxMap := make(map[*Subcluster]*Sandbox)
+	for oldSubclusterName, oldSandboxName := range oldSubclusterInSandbox {
+		_, oldSubclusterInNewSandboxes := newSuclusterInSandbox[oldSubclusterName]


You need to check if the old sandbox name and new sandbox name are the same. If we move a subcluster from a sandbox to another in new vdb, that subcluster is still in new vdb, but it will be unsandboxed.

I will make code change to cover that scenario and set up a new test case.

cchen-vertica · 2024-11-06T13:49:52Z

api/v1/verticadb_webhook.go

+	sandboxIndexMap := v.GenSandboxIndexMap()
+	for oldSubcluster, oldSandbox := range subclustersToUnsandbox {
+		oldSubclusterIndex, found := statusSClusterIndexMap[oldSubcluster.Name]
+		if oldSubcluster.Shutdown || found && v.Status.Subclusters[oldSubclusterIndex].Shutdown {


Add parenthesis for readability.

cchen-vertica · 2024-11-06T13:52:30Z

api/v1/verticadb_webhook.go

+			allErrs = append(allErrs, err)
+			continue
+		}
+		if oldSandbox.Shutdown {


For one sandbox, we probably reach here multiple times so we probably return duplicate errors.

cchen-vertica · 2024-11-06T13:55:37Z

api/v1/verticadb_webhook.go

+		newSandbox := newSandboxMap[sandboxName]
+		oldSandbox := oldSandboxMap[sandboxName]
+		if newSandbox.Shutdown {
+			for _, subclusterName := range oldSandbox.Subclusters {


Why are we checking old sandbox's subclusters? Doesn't this rule apply on the subclusters in new sandbox?

This is from the JIRA: " If a sandbox has shutdown(spec) true and any of its subclusters has the annotation "vertica.com/shutdown-driven-by-sandbox" set to true in the old vdb, we should prevent the user from updating that subcluster's shutdown field. "
Here we check the subclusters that exist in both new and old vdbs.

Then in line 1836, we should check oldSandbox.Shutdown.

No we should check newSandbox. Here is the scenario this covers: sandbox sand1 has a subcluster sc1. you set sand1.shutdown to true, the operator will set sc1.shutdown to true and add the annotation to sc1. So now sand.shutdown = true, sc1.shutdown. true and sc1 has "vertica.com/shutdown-driven-by-sandbox". We do not want a user to come and change sc1.shutdown because that annotation is a proof that the sandbox drives the shutdown.

This is relevant only if sand1.shutdown is still true(newvdb)

cchen-vertica · 2024-11-06T14:00:05Z

api/v1/verticadb_webhook.go

+	return allErrs
+}
+
+// checkSboxForShutdown checks if sbox or its scluster has shutdown field set to true in spec/status


Enrich the comment to explain the returned value.

cchen-vertica · 2024-11-07T19:05:16Z

api/v1/verticadb_webhook.go

+func (v *VerticaDB) findPersistSandboxes(oldObj *VerticaDB) []string {
+	oldSandboxes := oldObj.Spec.Sandboxes
+	newSandboxes := v.Spec.Sandboxes
+	oldSandboxMap := make(map[string]int)


This needs to be addressed.

cchen-vertica · 2024-11-07T19:06:46Z

api/v1/verticadb_webhook.go

+	return sclusterSboxMap
+}
+
+// checkUnsandboxShutdownConditions ensures we will not unsandbox a subcluster that has shutdown set to true, or a subcluster


Fix golint error of the comments here.

cchen-vertica · 2024-11-07T19:15:43Z

api/v1/verticadb_webhook.go

+		}
+		if oldSandbox.Shutdown {
+			i := sandboxIndexMap[oldSandbox.Name]
+			if i != unsandboxIndex { // this is to avoid duplicate error messages


Will this fix the duplicate error messages bug? In the for loop, we have this sequence: sc1, sand1; sc2, sand2; sc3, sand1. unsandboxIndex will be 1(assume sand1's index is 1), 2, 1. We still come here twice for sand1.

You should check the sandbox outside this loop or use a map to store checked sandboxes.

cchen-vertica · 2024-11-07T19:24:54Z

api/v1/verticadb_webhook.go

+		_, oldSubclusterInPersist := newSubclusterMap[oldSubclusterName]
+		oldSandbox := oldSandboxMap[oldSandboxName]
+		// for unsandboxing, check shutdown field of the subcluster and sandbox
+		if oldSubclusterInPersist {


This condition doesn't help. Even the subcluster does not persist, the subcluster could be unsandboxed.

cchen-vertica · 2024-11-07T19:33:12Z

api/v1/verticadb_webhook.go

+		newSandbox := newSandboxMap[sandboxName]
+		oldSandbox := oldSandboxMap[sandboxName]
+		if newSandbox.Shutdown {
+			for _, subclusterName := range oldSandbox.Subclusters {


Then in line 1836, we should check oldSandbox.Shutdown.

cchen-vertica · 2024-11-07T20:22:07Z

api/v1/verticadb_webhook.go

+	return allErrs
+}
+
+// checkTerminatingSandboxes ensure the sandbox to terminate is not shut down


This comment doesn't match the function implementation. I don't think we need to check if all subclusters inside the sandbox are terminating. If a "shutdown" sandbox is removed, we should error out.

That's a good point. I overthought this. @LiboYu2 when a sandbox is removed check its shutdown and if it is true, block the change. We should also do the same for remove_subcluster, a subcluster must not be removed from vdb if shutdown is true, but that will be part of his other ticket. This has already taken too long.

I will change the code to adopt the new logic. Could you update the rule in the JIRA as well?

roypaulin · 2024-11-07T21:03:15Z

api/v1/verticadb_webhook.go

+	allErrs = v.checkShutdownSandboxImage(old, allErrs)
+	allErrs = v.checkTerminatingSandboxes(old, allErrs)
+	allErrs = v.checkUnsandboxShutdownConditions(old, allErrs)
+	allErrs = v.checkAnnotatedSubclustersInShutdownSandbox(old, allErrs)
+	allErrs = v.checkNewSBoxOrSClusterShutdownUnset(allErrs)


Directly pass oldObj to these functions to stay consistent.

roypaulin · 2024-11-08T16:39:32Z

api/v1/verticadb_webhook.go

@@ -1828,20 +1828,18 @@ func (v *VerticaDB) checkAnnotatedSubclustersInShutdownSandbox(oldObj *VerticaDB
 	for sandboxName := range persistSandboxes {
 		newSandbox := newSandboxMap[sandboxName]
 		oldSandbox := oldSandboxMap[sandboxName]
-		if newSandbox.Shutdown {
+		if newSandbox.Shutdown { // !oldSandbox.Shutdown &&


Why this comment? I do not think oldSandbox.shutdown value matters.

This is useless. I will remove it.

roypaulin · 2024-11-08T16:40:56Z

api/v1/verticadb_webhook.go

-						allErrs = append(allErrs, err)
-					}
+				newSubcluster, oldSclusterPersist := newSubclusterMap[subclusterName.Name]
+				if oldSclusterPersist && oldSubcluster.Shutdown != newSubcluster.Shutdown && !newSubcluster.Shutdown {


Do you need the last part of this condition? We want to prevent any change.

Do you mean this one "!newSubcluster.Shutdown"? It is used to check if a user sets the shutdown to false while the sandbox is marked for shutdown. It is needed.

We want to prevent any kind of change so "oldSubcluster.Shutdown != newSubcluster.Shutdown" is enough

If we only have "oldSubcluster.Shutdown != newSubcluster.Shutdown", that will stop the operator from the setting the shutdown field to true from false.

cchen-vertica · 2024-11-08T16:43:31Z

api/v1/verticadb_webhook.go

@@ -1833,20 +1828,18 @@ func (v *VerticaDB) checkAnnotatedSubclustersInShutdownSandbox(old runtime.Objec
 	for sandboxName := range persistSandboxes {
 		newSandbox := newSandboxMap[sandboxName]
 		oldSandbox := oldSandboxMap[sandboxName]
-		if newSandbox.Shutdown {
+		if newSandbox.Shutdown { // !oldSandbox.Shutdown &&


Remove the comment in this line.

cchen-vertica · 2024-11-08T16:44:10Z

api/v1/verticadb_webhook.go

@@ -1822,8 +1818,7 @@ func (v *VerticaDB) checkUnsandboxShutdownConditions(old runtime.Object, allErrs

 // checkAnnotatedSubclustersInShutdownSandbox ensures: when a sandbox is to be shutdown, if a subcluster in it has annotation


Update the comment here

cchen-vertica · 2024-11-08T16:44:54Z

api/v1/verticadb_webhook.go

@@ -1833,20 +1828,18 @@ func (v *VerticaDB) checkAnnotatedSubclustersInShutdownSandbox(old runtime.Objec
 	for sandboxName := range persistSandboxes {


Why do we need the sandbox to be persistent?

Because of this condition "oldSubcluster.Shutdown != newSubcluster.Shutdown"
The rule applies when a user tries to mess with an existing subcluster. If it is a new subcluster, it will be covered by other rules.

That condition is for persistent subclusters, not for persistent sandboxes.

If the sandbox is not persistent, it will be a new sandbox, For new sandboxes to be created, its shutdown field will not be true. We are only concerned about subclusters in persistent sandboxes.

If we have a subcluster that isn't in a sandbox of old vdb, but we put it in the sandbox of new vdb, your code cannot cover that subcluster, right?

Do we have a rule to prevent a "shutdown" subcluster to add to a sandbox in your PR?

Yes. There are more rules in https://jira.verticacorp.com/jira/browse/VER-97254

Correct myself. Rule 1 of https://jira.verticacorp.com/jira/browse/VER-97368 covers that scenario.

If we have a subcluster that isn't in a sandbox of old vdb, but we put it in the sandbox of new vdb, your code cannot cover that subcluster, right?

Right. That is the sandboxing scenario. It will be covered in https://jira.verticacorp.com/jira/browse/VER-97254

cchen-vertica · 2024-11-08T16:47:20Z

api/v1/verticadb_webhook.go

-		}
-		// sandboxToBeRemoved and all its subclusters are not found in new spec
-		if sclustersToTerminate == numOfSclustersInSbox {
+		} else {


You can remove the else here since you already used a "continue" in line 1884

with the "else", it is easier to read and understand the logic.

Then line 1884 doesn't make sense. You can just use if oldSandboxMap[sandboxToBeRemoved].Shutdown, what's the point to use an else block.

I will remove it.

LiboYu2 added 22 commits October 24, 2024 09:51

implementation ready

adf92b3

remove duplicate checking

65d98ec

no regression at this point

0ff79ad

two create test cases added

96879d5

add annotatated subcluster test

c43e823

made some logic change

2c690bc

more tests need to be added

5f81aed

test case need conformation

56ea391

rewording some tests

7dca0d4

minor refactoring

df5724d

remove an edge test as confirmed by Roy

b7814e7

fix a bug

7511fa3

minor refactoring

fbd7d10

fix a bug found during manual testing

2f29519

fix bugs found

766c31b

not allow users to add drivenbyshutdown annotation

754635d

fix a typo

7d6a6a3

not allowed to unset scluter shutdown when sbox shutdown

c9c6826

add new unsandbox test case

6348299

distinguish unsandbox/terminate

d398cf3

remove newly added and uncommented previous implementation

7be1995

update annotated test cases

0c4d152

LiboYu2 requested review from roypaulin, cchen-vertica, fenic-fawkes, HaoYang0000 and qindotguan as code owners October 28, 2024 16:44

fix lint errors

c5df20c

roypaulin reviewed Oct 30, 2024

View reviewed changes

LiboYu2 and others added 4 commits October 31, 2024 11:57

Update api/v1/verticadb_webhook.go

136a6c0

Co-authored-by: Roy Paulin <[email protected]>

Update api/v1/verticadb_webhook.go

f7b12e0

Co-authored-by: Roy Paulin <[email protected]>

fix build errors

1eae0a2

Update api/v1/verticadb_webhook.go

24c1533

Co-authored-by: Roy Paulin <[email protected]>

roypaulin reviewed Nov 4, 2024

View reviewed changes

LiboYu2 and others added 4 commits November 4, 2024 16:03

Update api/v1/verticadb_webhook.go

bbfef12

Co-authored-by: Roy Paulin <[email protected]>

Update api/v1/verticadb_webhook.go

a554cda

Co-authored-by: Roy Paulin <[email protected]>

make changed per code review

801ecf6

Merge branch 'VER-97368-webhook-rules' of github.com:vertica/vertica-…

5a3f913

…kubernetes into VER-97368-webhook-rules

roypaulin reviewed Nov 5, 2024

View reviewed changes

address code review comments

6a69ce8

cchen-vertica reviewed Nov 6, 2024

View reviewed changes

LiboYu2 added 2 commits November 6, 2024 11:32

add new test cases for new unsandbox scenario

f0d1e86

optimize a little bit per code review

8bfe4a1

cchen-vertica reviewed Nov 7, 2024

View reviewed changes

roypaulin reviewed Nov 7, 2024

View reviewed changes

LiboYu2 added 4 commits November 7, 2024 17:18

fix lint error, duplicate err msg, logic change

4d4bea3

put back pieces removed by mistake

901a0e1

updated unsandbox definition per code review

b88741b

new rule to avoid user changing subcluster shutdown

a2715ed

roypaulin reviewed Nov 8, 2024

View reviewed changes

roypaulin requested a review from cchen-vertica November 8, 2024 16:41

cchen-vertica reviewed Nov 8, 2024

View reviewed changes

LiboYu2 added 3 commits November 8, 2024 11:49

remove useless comment

ae518fb

update method description

ec040a6

changed if statement

ae0a3c7

cchen-vertica approved these changes Nov 8, 2024

View reviewed changes

LiboYu2 merged commit 265f01a into main Nov 8, 2024
39 checks passed

LiboYu2 deleted the VER-97368-webhook-rules branch November 8, 2024 20:15

		allErrs = v.hasNoShutdownSubclusters(allErrs)
		allErrs = v.hasNoShutdownSandboxes(allErrs)

	return fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name)
	errMsgs = append(errMsgs, fmt.Sprintf("Shutdown field of sandbox %q is set to true", newSandbox.Name))

	return fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name)
	errMsgs = append(errMsgs, fmt.Sprintf("Subcluster %q is marked for shut down or has already been shut down", subcluster.Name))

	func (v VerticaDB) checkSboxForShutdown(newSandbox Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) string {
	func (v VerticaDB) checkSboxForShutdown(newSandbox Sandbox, newSClusterMap map[string]*Subcluster, statusSClusterIndexMap map[string]int) []string {
	errMsgs := []string{}

	fmt.Sprintf("cannot change Shutdown field for subcluster %q that has annotation \"vertica.com/shutdown-driven-by-sandbox\" set to true",
	fmt.Sprintf("cannot change shutdown field for subcluster %q whose shutdown is driven by sandbox %q",

	err := field.Invalid(p,
	err := field.Invalid(p.Child("shutdown"),

	// ensures a new subcluster or sandbox to be added to a vdb has Shutdown field set to false
	// validateNewSBoxOrSClusterShutdownUnset ensures a new subcluster or sandbox to be added to a vdb has Shutdown field set to false

		@@ -1822,8 +1818,7 @@ func (v *VerticaDB) checkUnsandboxShutdownConditions(old runtime.Object, allErrs

		// checkAnnotatedSubclustersInShutdownSandbox ensures: when a sandbox is to be shutdown, if a subcluster in it has annotation

		@@ -1833,20 +1828,18 @@ func (v *VerticaDB) checkAnnotatedSubclustersInShutdownSandbox(old runtime.Objec
		for sandboxName := range persistSandboxes {

webhook rules for sandbox shutdown #971

webhook rules for sandbox shutdown #971

Conversation

LiboYu2 commented Oct 28, 2024

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

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

roypaulin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LiboYu2 Nov 4, 2024 • edited 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

roypaulin left a 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

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

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

LiboYu2 Nov 4, 2024 •

edited

Loading