Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix statefulset pod selector #954

Merged
merged 2 commits into from
Apr 22, 2024
Merged

Conversation

JamesOwenHall
Copy link
Contributor

What are you trying to accomplish with this PR?
When deploying a StatefulSet, deploys could sporadically be marked failed due to Krane monitoring the health of the current revision's pods, not the updated revision. This PR updates the logic to monitor the updated revision.

How is this accomplished?
StatefulSet#parent_of_pod? is the method that filters pods. I've updated it to select pods matching .status.updateRevision.

What could go wrong?
...

@@ -65,7 +65,7 @@ def desired_replicas
def parent_of_pod?(pod_data)
return false unless pod_data.dig("metadata", "ownerReferences")
pod_data["metadata"]["ownerReferences"].any? { |ref| ref["uid"] == @instance_data["metadata"]["uid"] } &&
@instance_data["status"]["currentRevision"] == pod_data["metadata"]["labels"]["controller-revision-hash"]
@instance_data["status"]["updateRevision"] == pod_data["metadata"]["labels"]["controller-revision-hash"]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to figure out whether updateRevision will be empty the first time a StatefulSet is deployed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it does the right thing and sets updatedRevision and currentRevision to the same thing if there is no revision history:

https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/statefulset/stateful_set_control.go#L269-L272

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, but as you pointed out, it looks like it should do the right thing.

@JamesOwenHall
Copy link
Contributor Author

🎩 'ed and seems to have fixed the issue.

@JamesOwenHall JamesOwenHall merged commit 9732191 into main Apr 22, 2024
65 checks passed
@JamesOwenHall JamesOwenHall deleted the fix-statefulset-pod-selector branch April 22, 2024 14:58
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.

None yet

3 participants