Refactor Pod await logic #590

lblackstone · 2019-06-10T20:38:29Z

Fixes #450

metral

Overall LGTM. Let's get some other eyes on here to review given the size of the PR

pkg/await/core_pod.go

pkg/await/fixtures/pod.go

pkg/await/states/pod_test.go

hausdorff

It is with great regret that I must report this is vastly superior to what I originally wrote.

In fact, just today I remarked to @lukehoban that you occasionally hear stories of people writing some random Go and wishing they had a feature of another language, only to rewrite it and find out (a) they didn't need it, and (b) it's simpler without it. I told Luke I had never had this experience and I couldn't tell whether that's because I'm a really bad Go programmer or not.

Well, now that we've rewritten it, the jury is in: I am just a bad Go programmer.

Anyway, a few questions/suggestions below.

hausdorff · 2019-07-03T20:54:18Z

pkg/await/apps_deployment_test.go

@@ -497,7 +497,7 @@ func Test_Apps_Deployment(t *testing.T) {
 				object: deploymentProgressing(inputNamespace, deploymentInputName, revision1),
 				subErrors: []string{
 					"Minimum number of live Pods was not attained",
-					`1 Pods failed to run because: [ImagePullBackOff] Back-off pulling image "sdkjlsdlkj"`,
+					`containers with unready status: [nginx] -- Back-off pulling image "sdkjlsdlkj"`,


Not a huge thing but as a general rule I think we should keep the ImagePullBackOff because the standard error codes are easier to Google. It's also easier for people who know how this stuff works to understand what's going on.

hausdorff · 2019-07-03T20:59:07Z

pkg/logging/types.go

+
+// Message stores a log string and the severity for the log message.
+type Message struct {
+	S        string


This is a great start, and it's ok if the answer is no -- but if there is additional structured information we can pull out of the S and into the Message struct, I think we should do that. Is there anything that seems likely to fit that?

Not off the top of my head. Will keep it in mind as I continue refactoring.

hausdorff · 2019-07-03T21:03:30Z

pkg/logging/types.go

+}
+
+// Empty returns true if the Message is uninitialized, false otherwise.
+func (m Message) Empty() bool {


Curious: why make Empty a method on Message instead of making Result#Message a pointer (in which case you could check if it's "empty" by checking whether it's nil).

Message is used in a bunch of places. Can't recall the specific reason, but it was easier this way when I wrote it.

hausdorff · 2019-07-03T21:14:57Z

pkg/await/apps_statefulset.go

-	scheduleErrorCounts := map[string]int{}
-	containerErrorCounts := map[string]int{}
-	for _, pod := range sia.pods {
+func (sia *statefulsetInitAwaiter) aggregatePodErrors() logging.Messages {


Not a big deal, but an alternative design you might consider here is to have it take a lambda as an argument instead of passing an allocated list back, and then allocating another list, again, in response, which is what we're doing in a lot of the callsites.

messages := sia.aggregatePodErrors(func() { sia.config.logMessage(message) })

hausdorff · 2019-07-03T21:16:11Z

pkg/await/apps_deployment.go

 		}
+		messages = append(messages, checker.Update(pod).Warnings()...)


hausdorff · 2019-07-03T21:36:06Z

pkg/await/fixtures/pod.go

@@ -61,3 +62,149 @@ func PodBasic() *podBasic {
 		},
 	}
 }
+
+// PodBase returns Pod struct with basic data initialized.
+func PodBase(name, namespace string) *corev1.Pod {


Why build the pods up explicitly in code instead of just deserializing JSON? Seems like it's advantageous to have the verbatim resource definitions as the basis of these tests, and as it's written I can't quite tell if they are exactly correct. Also, the status field for pod in particular is way way fiddly. I consider the comment from @metral below evidence of all of this. :)

hausdorff · 2019-07-03T21:44:06Z

pkg/await/states/pod.go

+
+func NewPodChecker() *StateChecker {
+	return &StateChecker{
+		conditions: []Condition{podScheduled, podInitialized, podReady},


lol this is way better than the way we're doing it now.

hausdorff · 2019-07-03T21:55:36Z

pkg/await/states/pod.go

+	result := Result{Description: fmt.Sprintf("Waiting for Pod %q to be scheduled", fqName(pod))}
+
+	for _, condition := range pod.Status.Conditions {
+		if condition.Type == v1.PodScheduled {


This loop-and-check-type logic seems error-prone, and I think we should consider moving this into its own function, something like:

result := Result{Description: fmt.Sprintf("Waiting for Pod %q to be scheduled", fqName(pod))} matchCondition( pod.Status.Conditions, v1.PodScheduled, func(v1.PodCondition) { result.Ok = true }, // onTrue func(condition v1.PodCondition) { msg := statusFromCondition(condition) if len(msg) > 0 { result.Message = logging.StatusMessage(msg) } }) // onFalse

Same thing, perhaps, for container status?

I added a filter function to flatten out the logic, and structured the conditional checking similarly to your suggestion.

hausdorff · 2019-07-03T22:01:05Z

pkg/await/states/types.go

+	"github.com/pulumi/pulumi-kubernetes/pkg/logging"
+)
+
+// Result specifies the result of a Condition applied to an input object.


Might want to mention that this is a v1.PodCondition, and write a bit more about what this is actually for -- e.g., it's the Result of a check that a Pod has some v1.PodCondition that is true, with some metadata useful for rendering messages about it.

This actually isn't specific to PodConditions; it's a generic condition that is used to determine readiness of the relevant resource.

hausdorff · 2019-07-03T22:02:10Z

pkg/await/states/types.go

+// Condition is a function that checks a state and returns a Result.
+type Condition func(s interface{}) Result
+
+// StateChecker holds the data required to generically implement await logic.


Might be nice to make this clearer, too. StateChecker checks that a Pod has a set of v1.PodConditions that match the desired state.

Again, StateChecker isn't specific to Pod; it will be used for every resource type once I'm done with refactoring.

lblackstone force-pushed the lblackstone/pod-await-refactor branch 7 times, most recently from a5cdb43 to a01c1e0 Compare June 26, 2019 22:27

lblackstone marked this pull request as ready for review June 27, 2019 03:10

lblackstone force-pushed the lblackstone/pod-await-refactor branch from 448a412 to 30e329d Compare June 27, 2019 03:14

lblackstone requested review from hausdorff and metral June 27, 2019 19:38

metral approved these changes Jul 1, 2019

View reviewed changes

pkg/await/core_pod.go Show resolved Hide resolved

pkg/await/fixtures/pod.go Show resolved Hide resolved

pkg/await/states/pod_test.go Show resolved Hide resolved

lblackstone added 6 commits July 2, 2019 09:37

Refactor Pod await logic

549a92b

Update tests

8454a8c

Finish testing

c0530c2

Fix unit test error messages

99499c2

Cleanup

2c1181b

Address Mike's feedback

d5a20f3

lblackstone force-pushed the lblackstone/pod-await-refactor branch from 2970ee2 to d5a20f3 Compare July 2, 2019 15:58

lblackstone merged commit 46b1bce into master Jul 3, 2019

pulumi-bot deleted the lblackstone/pod-await-refactor branch July 3, 2019 19:54

lblackstone added a commit that referenced this pull request Jul 3, 2019

Move changelog entry for #590 to correct release

a9dd686

lblackstone added a commit that referenced this pull request Jul 3, 2019

Move changelog entry for #590 to correct release (#620)

32349c4

hausdorff reviewed Jul 3, 2019

View reviewed changes

lblackstone mentioned this pull request Jul 9, 2019

Respond to Pod await PR feedback #626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Pod await logic #590

Refactor Pod await logic #590

lblackstone commented Jun 10, 2019

metral left a comment

hausdorff left a comment

hausdorff Jul 3, 2019

hausdorff Jul 3, 2019

lblackstone Jul 9, 2019

hausdorff Jul 3, 2019

lblackstone Jul 9, 2019

hausdorff Jul 3, 2019

hausdorff Jul 3, 2019

hausdorff Jul 3, 2019

hausdorff Jul 3, 2019

hausdorff Jul 3, 2019

lblackstone Jul 9, 2019

hausdorff Jul 3, 2019

lblackstone Jul 9, 2019

hausdorff Jul 3, 2019

lblackstone Jul 9, 2019

		}
		messages = append(messages, checker.Update(pod).Warnings()...)

Refactor Pod await logic #590

Refactor Pod await logic #590

Conversation

lblackstone commented Jun 10, 2019

metral left a comment

Choose a reason for hiding this comment

hausdorff 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