Skip to content

Conversation

@hessjcg
Copy link
Collaborator

@hessjcg hessjcg commented Nov 18, 2022

The proxy configuration is now only applied when a pod is created. The old algorithm allowed updates to the PodSpec
to apply changes after the pod was created. This PR simplifies the logic that applies the proxy configuration to a
PodSpec, mainly removing the code that would update the pod spec multiple times.

@hessjcg hessjcg changed the title reconcile podtemplate updates feat: PodSpec changes now are applied only to pods on creation (#41 step 7) Nov 18, 2022
@hessjcg hessjcg force-pushed the reconcile-podtemplate-updates branch from 975ee9b to 015e334 Compare November 18, 2022 18:31
@hessjcg hessjcg force-pushed the reconcile-podtemplate-updates branch 9 times, most recently from 77cbf0b to 7cf47c4 Compare December 1, 2022 19:15
@hessjcg hessjcg force-pushed the reconcile-controller-updates branch from b9d6eb1 to 309d711 Compare December 1, 2022 19:15
@hessjcg hessjcg force-pushed the reconcile-podtemplate-updates branch 18 times, most recently from 6916456 to ab25dd5 Compare December 6, 2022 00:07
@hessjcg hessjcg changed the base branch from reconcile-controller-updates to main December 6, 2022 00:08

}

func TestProperCleanupOfEnvAndVolumes(t *testing.T) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't remove the a configuration from a PodTemplateSpec after it has been applied anymore.

u = workload.NewUpdater()
)

// Create a deployment
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the changes in this test simply replace Deployment with Pod.

wantsInstanceArg, foundContainer.Args)
}

// Now change the spec
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to test applying the configuration to the PodSpec a second time with a slight change.

})
}

// loadOldEnvVarState loads the state connecting EnvVar changes done by the
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove code that saves the the existing state so changes can later be removed.

inst := matches[i]
var instContainer *corev1.Container

for j := range containers {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the beginning of this function all containers on PodSpec were defined in the workload by the user. No more need to distinguish between containers that existed in the original workload and containers added by the operator.

@hessjcg hessjcg marked this pull request as ready for review December 6, 2022 00:16
@hessjcg hessjcg requested a review from a team December 6, 2022 00:16
@hessjcg hessjcg force-pushed the reconcile-podtemplate-updates branch from 5f0429a to 71493f9 Compare December 6, 2022 00:20
for i := 0; i < len(proxies); i++ {
l.Items[i] = *proxies[i]
}
_, _, err := u.ConfigurePodProxies(l, wl, nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only place ConfigurePodProxies is called. Should we get rid of the unused return values from the signature?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return values are used in mainline code coming in the next PR.

// internal/controller/pod_controller.go:88

// Reconfigure the PodSpec
updated, matchingInstances, wlConfigErr = a.updater.ConfigurePodProxies(instList, wl, owners)

Sadly, that means we need this awkward conversion method in the test code.

@hessjcg hessjcg merged commit df1f322 into main Dec 6, 2022
@hessjcg hessjcg deleted the reconcile-podtemplate-updates branch December 13, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants