Adding support for daemonSet and statefulSet resources in charts #405

dcurran90 · 2023-08-10T19:31:38Z

Update waitForDeployments to waitForWorkloads and add the ability to wait for DaemonSets and StatefulSets.
Add tests for this new feature and increase code coverage

Closes #286

mgoerens

Looks good overall, thanks ! I've added some small suggestions.

mgoerens · 2023-08-17T15:43:54Z

internal/tool/kubectl.go

 				// Just after rollout, pods from the previous deployment revision may still be in a
 				// terminating state.


I don't really get this comment. Here we check if the pods have become available, and I don't really what this has to do with a previous deployment. Wdyt ? Should we remove/update it ?

good call. I'll double check all the comments and make them more relevant.

mgoerens · 2023-08-17T15:47:15Z

internal/tool/kubectl.go

 			}
 		}
 	}

-	if len(getDeploymentsError) > 0 {
-		errorMsg := fmt.Sprintf("Time out retrying after %s", getDeploymentsError)
+	if len(getWorkloadResourceError) > 0 {


Suggested change

if len(getWorkloadResourceError) > 0 {

if getWorkloadResourceError != "" {

or, longer but might show the intention a bit better:

Suggested change

if len(getWorkloadResourceError) > 0 {

if errDeployment != nil || errDaemonSets != nil || errStatefulSets != nil

internal/tool/kubectl_test.go

internal/tool/kubectl.go

komish · 2023-08-24T14:51:17Z

internal/tool/kubectl.go

+			unavailableWorkloadResources = []workloadNotReady{{Name: "none", ResourceType: "Deployment", Unavailable: 1}}
+			getWorkloadResourceError = fmt.Sprintf("error getting deployments from namespace %s : %v", namespace, errDeployments)
+			utils.LogWarning(getWorkloadResourceError)
+			time.Sleep(time.Second)


What's the purpose of sleeping here?

This was actually left over from what the function previously did.
I believe the idea is to avoid excessive calls and rate limiting since it'll immediately hit the API again after these failures. Happy to remove it if this is a non-issue but trying to find the default rate limit info first

komish · 2023-08-24T14:56:40Z

internal/tool/kubectl.go

+		if errDeployments != nil {
+			unavailableWorkloadResources = []workloadNotReady{{Name: "none", ResourceType: "Deployment", Unavailable: 1}}
+			getWorkloadResourceError = fmt.Sprintf("error getting deployments from namespace %s : %v", namespace, errDeployments)
+			utils.LogWarning(getWorkloadResourceError)
+			time.Sleep(time.Second)
+		} else if errDaemonSets != nil {
+			unavailableWorkloadResources = []workloadNotReady{{Name: "none", ResourceType: "DaemonSet", Unavailable: 1}}
+			getWorkloadResourceError = fmt.Sprintf("error getting daemon sets from namespace %s : %v", namespace, errDaemonSets)
+			utils.LogWarning(getWorkloadResourceError)
+			time.Sleep(time.Second)
+		} else if errStatefulSets != nil {
+			unavailableWorkloadResources = []workloadNotReady{{Name: "none", ResourceType: "StatefulSet", Unavailable: 1}}
+			getWorkloadResourceError = fmt.Sprintf("error getting stateful sets from namespace %s : %v", namespace, errStatefulSets)
+			utils.LogWarning(getWorkloadResourceError)
 			time.Sleep(time.Second)


A few things for this block.

This feels like we should be dropping the else if component. Running through each if block separately should work here.

There's a good deal of repetition in what's being executed, indicating we could probably just refactor this to remove it and, in turn, be more concise.

refactored to reduce repetition

komish · 2023-08-24T14:58:48Z

internal/tool/kubectl.go

+			getWorkloadResourceError = ""
+			// Check the number of unavailable replicas for each workload type
 			for _, deployment := range deployments {
-				// Just after rollout, pods from the previous deployment revision may still be in a
-				// terminating state.
 				if deployment.Status.UnavailableReplicas > 0 {
-					unavailableDeployments = append(unavailableDeployments, deploymentNotReady{Name: deployment.Name, Unavailable: deployment.Status.UnavailableReplicas})
+					unavailableWorkloadResources = append(unavailableWorkloadResources, workloadNotReady{Name: deployment.Name, ResourceType: "Deployment", Unavailable: deployment.Status.UnavailableReplicas})
+				}
+			}
+			for _, daemonSet := range daemonSets {
+				if daemonSet.Status.NumberUnavailable > 0 {
+					unavailableWorkloadResources = append(unavailableWorkloadResources, workloadNotReady{Name: daemonSet.Name, ResourceType: "DaemonSet", Unavailable: daemonSet.Status.NumberUnavailable})
+				}
+			}
+			for _, statefulSet := range statefulSets {
+				// StatefulSet doesn't report unavailable replicas so it is calculated here
+				unavailableReplicas := statefulSet.Status.Replicas - statefulSet.Status.AvailableReplicas
+				if unavailableReplicas > 0 {
+					unavailableWorkloadResources = append(unavailableWorkloadResources, workloadNotReady{Name: statefulSet.Name, ResourceType: "StatefulSet", Unavailable: unavailableReplicas})
 				}
 			}
-			if len(unavailableDeployments) > 0 {
-				utils.LogInfo(fmt.Sprintf("Wait for %d deployments:", len(unavailableDeployments)))
-				for _, unavailableDeployment := range unavailableDeployments {
-					utils.LogInfo(fmt.Sprintf("    - %s with %d unavailable replicas", unavailableDeployment.Name, unavailableDeployment.Unavailable))
+
+			// If any pods are unavailable report it and sleep until the next loop
+			// If everythign is available exit the loop
+			if len(unavailableWorkloadResources) > 0 {
+				utils.LogInfo(fmt.Sprintf("Wait for %d workload resources:", len(unavailableWorkloadResources)))
+				for _, unavailableWorkloadResource := range unavailableWorkloadResources {
+					utils.LogInfo(fmt.Sprintf("    - %s %s with %d unavailable pods", unavailableWorkloadResource.ResourceType, unavailableWorkloadResource.Name, unavailableWorkloadResource.Unavailable))
 				}
 				time.Sleep(time.Second)
 			} else {
-				utils.LogInfo(fmt.Sprintf("Finish wait for deployments, --timeout time left %s", time.Until(deadline).String()))
+				utils.LogInfo(fmt.Sprintf("Finish wait for workload resources, --timeout time left %s", time.Until(deadline).String()))


The structure here feels like we can abstract out the parsing of these into separate functions and call them.

Perhaps there's an opportunity to use goroutines here to let the parsing of statefulsets, deployments, and daemonsets work independently of each other? WDYT. I think it could do away with the sleeps I'm seeing.

komish · 2023-08-24T15:01:40Z

internal/tool/kubectl.go

+		deployments, errDeployments := listDeployments(k, context, namespace, selector)
+		daemonSets, errDaemonSets := listDaemonSets(k, context, namespace, selector)
+		statefulSets, errStatefulSets := listStatefulSets(k, context, namespace, selector)


So I'm understanding that we check for these three resources regardless of whether the deployed chart uses them? The idea being that charts that don't use statefulsets/daemonsets would just auto-pass the "readiness" check because none of those workloads exist. Is that a correct understanding?

That's fine, I just want to make sure. Fundamentally, this check seems to be flawed because charts can deploy all sorts of resources. But at the end of the day, this is a better check than nothing.

yes, charts that don't try do deploy certain resources will just auto-pass since we're not expecting them to be there.

As far as checking for other resources I could add in jobs at least because it seems like the only one we're missing from the list of default workload resources (https://kubernetes.io/docs/concepts/workloads/) but didn't know if i should since it wasn't requested and didn't want to go overboard. Happy to make another PR for that though if it's needed

komish

@dcurran90 Thanks for your work on this.

Few comments left. I think we can be a little more concise here, and doing so would preserve the readability of these bits. Let me know if I can help with anything.

komish · 2023-08-29T21:19:15Z

@dcurran90 just a heads up that tests are failing here, and should be reproducible to make test

komish

/lgtm

@mgoerens when you're happy with this, go ahead and merge away.

mgoerens · 2023-09-01T15:55:54Z

/lgtm

Adding support for daemonSet and statefulSet resources in charts

2ae34b8

dcurran90 mentioned this pull request Aug 10, 2023

Add support for testing daemon and statefulsets #286

Closed

Fixing formatting

2af321c

dcurran90 force-pushed the workloadResources branch from e2bcba9 to 2af321c Compare August 16, 2023 15:40

mgoerens requested changes Aug 17, 2023

View reviewed changes

adding comments and cleaning up functions

380f7c2

dcurran90 requested a review from mgoerens August 17, 2023 21:30

re-run tests

50701f3

komish reviewed Aug 24, 2023

View reviewed changes

komish requested changes Aug 24, 2023

View reviewed changes

refactoring WaitForWorkloadResources and adding comments

09fc7af

fixing test errors

562919e

komish approved these changes Aug 31, 2023

View reviewed changes

mgoerens approved these changes Sep 1, 2023

View reviewed changes

mgoerens merged commit ab27c64 into redhat-certification:main Sep 1, 2023
5 checks passed

mgoerens mentioned this pull request Sep 13, 2023

Adjust expected timeout message and accept regex in expected message openshift-helm-charts/development#256

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for daemonSet and statefulSet resources in charts #405

Adding support for daemonSet and statefulSet resources in charts #405

dcurran90 commented Aug 10, 2023 •

edited

Loading

mgoerens left a comment

mgoerens Aug 17, 2023

dcurran90 Aug 17, 2023

mgoerens Aug 17, 2023

komish Aug 24, 2023

dcurran90 Aug 29, 2023

komish Aug 24, 2023

dcurran90 Aug 29, 2023

komish Aug 24, 2023

komish Aug 24, 2023

dcurran90 Aug 29, 2023

komish left a comment

komish commented Aug 29, 2023

komish left a comment

mgoerens commented Sep 1, 2023

		// Just after rollout, pods from the previous deployment revision may still be in a
		// terminating state.

	if len(getWorkloadResourceError) > 0 {
	if getWorkloadResourceError != "" {

	if len(getWorkloadResourceError) > 0 {
	if errDeployment != nil \|\| errDaemonSets != nil \|\| errStatefulSets != nil

Adding support for daemonSet and statefulSet resources in charts #405

Adding support for daemonSet and statefulSet resources in charts #405

Conversation

dcurran90 commented Aug 10, 2023 • edited Loading

mgoerens 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

komish left a comment

Choose a reason for hiding this comment

komish commented Aug 29, 2023

komish left a comment

Choose a reason for hiding this comment

mgoerens commented Sep 1, 2023

dcurran90 commented Aug 10, 2023 •

edited

Loading