Ver 97254 shutdown webhook #979

LiboYu2 · 2024-11-12T18:57:45Z

Here are the rules implemented in this PR:

you cannot sandbox a subcluster with shutdown true(spec or status).
you cannot add a subcluster to a sandbox that has the shutdown field set to true
you cannot add a subcluster to a sandbox that already has at least one subcluster with shutdown true
you cannot scale (up/down) a subcluster with shutdown true(spec or status)
You cannot remove a subcluster with shutdown true(spec or status)

cchen-vertica · 2024-11-13T19:35:29Z

api/v1/verticadb_webhook.go

+	statusSclusterIndexMap := v.GenStatusSClusterIndexMap()
+	oldSclusterMap := oldObj.GenSubclusterMap()
+	newSclusterIndexMap := v.GenSubclusterIndexMap()
+	for subclusterName, sandboxName := range newSclusterSboxMap {


This loop will only check subclusters in the sandbox. What if the subclusters not in a sandbox?

Withdraw my previous comment. I thought you were talking about the other function. You are right. I will make change to adopt that scenario.

Isn't this function for this rule: you cannot scale (up/down) a subcluster with shutdown true(spec or status)?

This will not include subclusters in the main cluster. You should iterate over new.spec.subclusters, for each subcluster , check if size has changed, if true, check if shutdown(spec/status) is true, if true append an error, then check if it is part of a sandbox, if true check sandbox.Shutdown, if true, append an error

Rectification, given that there is already a rule that prevents scaling a subcluster that is part of a sandbox, you do not need to check the sandbox.

cchen-vertica · 2024-11-13T19:40:08Z

api/v1/verticadb_webhook.go

+	newSandboxMap := v.GenSandboxMap()
+	statusSclusterIndexMap := v.GenStatusSClusterIndexMap()
+	sandboxWithError := map[string]bool{}
+	for subclusterName, sandboxName := range newSclusterSboxMap {


Minor: rename subclusterName to newSC, sandboxName to newSB for bettern readability.

cchen-vertica · 2024-11-13T19:41:52Z

api/v1/verticadb_webhook.go

+		// or an existing subcluster (found in status), the latter of which includes two scenarios:
+		//   1 the subcluster was in a sandbox (whose name is different from current sandbox name)
+		//   2 the subcluster was not in a sandbox previously
+		if !isInOldSandbox || isInOldSandbox && oldSandboxName != sandboxName {


Use parenthesis to separate the conditions for readability.

The case isInOldSandbox && oldSandboxName != sandboxName is for unsandboxing. I remember this is checked in another rule. Will we get two errors for this case from two different rules?

Good memory! Here is the related rule from my previous PR:
You cannot unsandbox a subcluster with shutdown true(spec and/or status) or part of a sandbox with shutdown true

There will be two error messages:
When a subcluster is moved from one sandbox to another,

this related rule will raise an error for unsandboxing scenario.

The rule implmented in this PR will raise another error messages for sandboxing scenario.

Removed this check "isInOldSandbox && oldSandboxName != sandboxName" per offline discussion to avoid duplicate error messages.

cchen-vertica · 2024-11-14T17:33:27Z

api/v1/verticadb_webhook.go

+	statusSclusterIndexMap := v.GenStatusSClusterIndexMap()
+	oldSclusterMap := oldObj.GenSubclusterMap()
+	newSclusterIndexMap := v.GenSubclusterIndexMap()
+	for subclusterName, sandboxName := range newSclusterSboxMap {


Isn't this function for this rule: you cannot scale (up/down) a subcluster with shutdown true(spec or status)?

cchen-vertica · 2024-11-14T17:38:45Z

api/v1/verticadb_webhook.go

-		oldSandboxName, isInOldSandbox := statusScluterSboxMap[subclusterName]
-		_, foundSubcluster := newSclusterMap[subclusterName]
+	for newSC, newSB := range newSclusterSboxMap {
+		_, foundSubcluster := newSclusterMap[newSC]
 		if !foundSubcluster {
 			continue // avoid panic. this error should have been reported by other functions
 		}
 		// the subcluster to be sandboxed is either a new subcluster to be added (not found in status)
 		// or an existing subcluster (found in status), the latter of which includes two scenarios:
 		//   1 the subcluster was in a sandbox (whose name is different from current sandbox name)


Remove this line and modify the comment accordingly.

cchen-vertica · 2024-11-14T17:40:38Z

api/v1/verticadb_webhook.go

 		if !foundSubcluster {
 			continue // avoid panic. this error should have been reported by other functions
 		}
 		// the subcluster to be sandboxed is either a new subcluster to be added (not found in status)
 		// or an existing subcluster (found in status), the latter of which includes two scenarios:
 		//   1 the subcluster was in a sandbox (whose name is different from current sandbox name)
 		//   2 the subcluster was not in a sandbox previously
-		if !isInOldSandbox || isInOldSandbox && oldSandboxName != sandboxName {


We still need the condition if !isInOldSandbox to cover the case 2: the subcluster was not in a sandbox previously

After testing, I have to put back the condition "isInOldSandbox && oldSandboxName != sandboxName".
This is what I found:
When a subcluster is moved from sandbox A to sandbox B and A is shut down, , the previous rule kicks in.
When A is not shut down but B is shut down, the new rule applies.
Only when both A and B are shut down, there will be two different error messages: one from unsandboxing and one from sandboxing.

roypaulin · 2024-11-14T17:33:41Z

api/v1/verticadb_webhook.go

+	subclustersToBeRemoved := vutil.MapKeyDiff(oldSubclusterMap, newSubclusterMap)
+	for _, subclusterToBeRemoved := range subclustersToBeRemoved {
+		subclusterstatus, foundInStatus := statusSubclusterMap[subclusterToBeRemoved]
+		if oldSubclusterMap[subclusterToBeRemoved].Shutdown || foundInStatus && subclusterstatus.Shutdown {


minor

Suggested change

if oldSubclusterMap[subclusterToBeRemoved].Shutdown || foundInStatus && subclusterstatus.Shutdown {

if oldSubclusterMap[subclusterToBeRemoved].Shutdown || (foundInStatus && subclusterstatus.Shutdown) {

roypaulin · 2024-11-14T17:39:16Z

api/v1/verticadb_webhook.go

+	for _, subclusterToBeRemoved := range subclustersToBeRemoved {
+		subclusterstatus, foundInStatus := statusSubclusterMap[subclusterToBeRemoved]
+		if oldSubclusterMap[subclusterToBeRemoved].Shutdown || foundInStatus && subclusterstatus.Shutdown {
+			p := field.NewPath("spec").Child("subclusters")


Set this outside of the loop.

roypaulin · 2024-11-14T17:39:32Z

api/v1/verticadb_webhook.go

+		subclusterstatus, foundInStatus := statusSubclusterMap[subclusterToBeRemoved]
+		if oldSubclusterMap[subclusterToBeRemoved].Shutdown || foundInStatus && subclusterstatus.Shutdown {
+			p := field.NewPath("spec").Child("subclusters")
+			err := field.Invalid(p,


Suggested change

err := field.Invalid(p,

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

The subcluster is already removed. We cannot further drill down and give the index.

roypaulin · 2024-11-14T17:52:34Z

api/v1/verticadb_webhook.go

+	statusSclusterIndexMap := v.GenStatusSClusterIndexMap()
+	oldSclusterMap := oldObj.GenSubclusterMap()
+	newSclusterIndexMap := v.GenSubclusterIndexMap()
+	for subclusterName, sandboxName := range newSclusterSboxMap {


This will not include subclusters in the main cluster. You should iterate over new.spec.subclusters, for each subcluster , check if size has changed, if true, check if shutdown(spec/status) is true, if true append an error, then check if it is part of a sandbox, if true check sandbox.Shutdown, if true, append an error

LiboYu2 added 9 commits November 12, 2024 09:48

reuse code

bcf41ec

implemented the old rule

4901a4d

to change test cases next

0b538d0

use simpler method next

98aef2f

simplified condition check

ecb6c57

ready for manual testing

c9a0b0e

better error messages

949b76f

remove shutdown scluster check ready

cb3f77e

refine comment

0eb9e37

LiboYu2 requested review from roypaulin, cchen-vertica, fenic-fawkes, HaoYang0000 and qindotguan as code owners November 12, 2024 18:57

LiboYu2 added 2 commits November 13, 2024 10:27

fix issue found from VER-97368

86d26d0

fix lint complaints

f110852

cchen-vertica reviewed Nov 13, 2024

View reviewed changes

LiboYu2 added 2 commits November 13, 2024 15:17

minor change from code review

f42c03c

remove duplicate err msgs per code review

523c614

cchen-vertica reviewed Nov 14, 2024

View reviewed changes

roypaulin reviewed Nov 14, 2024

View reviewed changes

changes to address code review issues

3013c62

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ver 97254 shutdown webhook #979

Ver 97254 shutdown webhook #979

LiboYu2 commented Nov 12, 2024

cchen-vertica Nov 13, 2024

LiboYu2 Nov 13, 2024 •

edited

Loading

cchen-vertica Nov 14, 2024

roypaulin Nov 14, 2024

roypaulin Nov 15, 2024

cchen-vertica Nov 13, 2024

cchen-vertica Nov 13, 2024

cchen-vertica Nov 13, 2024

LiboYu2 Nov 13, 2024

LiboYu2 Nov 13, 2024

cchen-vertica Nov 14, 2024

cchen-vertica Nov 14, 2024

cchen-vertica Nov 14, 2024

LiboYu2 Nov 15, 2024

roypaulin Nov 14, 2024

roypaulin Nov 14, 2024

roypaulin Nov 14, 2024

LiboYu2 Nov 15, 2024

roypaulin Nov 14, 2024

	if oldSubclusterMap[subclusterToBeRemoved].Shutdown \|\| foundInStatus && subclusterstatus.Shutdown {
	if oldSubclusterMap[subclusterToBeRemoved].Shutdown \|\| (foundInStatus && subclusterstatus.Shutdown) {

Ver 97254 shutdown webhook #979

Are you sure you want to change the base?

Ver 97254 shutdown webhook #979

Conversation

LiboYu2 commented Nov 12, 2024

Choose a reason for hiding this comment

LiboYu2 Nov 13, 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

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 13, 2024 •

edited

Loading