Skip to content

fix: Handle StandalonePods Succeeded case when checking status #9580

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.

Sign up for GitHub

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

Jump to bottom
Merged
mattsanta merged 2 commits into from
Nov 25, 2024
Merged

fix: Handle StandalonePods Succeeded case when checking status #9580

mattsanta merged 2 commits into from
Nov 25, 2024

Conversation

@Y--
Copy link
Contributor

@Y-- Y-- commented Nov 21, 2024

Description
When Skaffold checks a standalone pod's status (method checkStandalonePodsStatus) , it currently only considers Failed and Running state, any other status will lead the pod to be systematically added to the pendingPods collection.

According to the documentation, a pod can be in 5 different states: Pending, Running, Succeeded, Failed or Unknown.

While it make sense for Pending or Unknown pods to be added to the pendingPods collection, I think it does not for the Succeeded ones (pods that terminated successfully)

This PR introduces a tiny change which just skips Succeeded pods.

Tests

I tried to look at other test and could only find:

  • unit tests on CheckStatus that don't involve StandalonePods resource
  • TestPollServiceStatus, TestPollJobStatus, TestPollDeployment, none of which involve StandalonePods resource

Please let me know if I've missed it, I'd be happy to add test on it.

Thanks!

@google-cla
Copy link

google-cla bot commented Nov 21, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Nov 21, 2024 •
edited
Loading

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@Y-- Y-- force-pushed the fix/ignore-succeeded-pods branch from 162cb2a to bb83f83 Compare November 21, 2024 13:56
@mattsanta
Copy link
Contributor

mattsanta commented Nov 21, 2024

Thanks for the PR!

So the current behavior is that status checking does not work if the Pod has completed and has a Succeeded phase, skaffold will continue to poll the Pod status until it times out. I also went ahead and tested your fix by building skaffold locally and it looks like it works.

I know the current tests don't ever use StandalonePods, but could you be the first one to add a test case that does for this scenario?

@Y-- Y-- force-pushed the fix/ignore-succeeded-pods branch 2 times, most recently from 0778a47 to 38245f5 Compare November 25, 2024 09:07
@Y-- Y-- force-pushed the fix/ignore-succeeded-pods branch from 38245f5 to 4e7875c Compare November 25, 2024 09:09
@Y--
Copy link
Contributor Author

Y-- commented Nov 25, 2024 •
edited
Loading

Thanks for the review @mattsanta ! I have added a few tests.
I also had to skip event publishing in CheckStatus for Succeeded pods, which was failing for these pods.

Also, I'm not sure why the conventional commit linter isn't happy with me... I must be missing something big, but I think I have a subject & type for each commit message?

@mattsanta mattsanta changed the title Handle StandalonePods Succeeded case when checking status fix: Handle StandalonePods Succeeded case when checking status Nov 25, 2024
@mattsanta
Copy link
Contributor

mattsanta commented Nov 25, 2024

Thanks for the review @mattsanta ! I have added a few tests. I also had to skip event publishing in CheckStatus for Succeeded pods, which was failing for these pods.

Also, I'm not sure why the conventional commit linter isn't happy with me... I must be missing something big, but I think I have a subject & type for each commit message?

It's referring to the title of the PR. I went ahead and added fix: prefix to it :)

@mattsanta mattsanta merged commit bd02eac into GoogleContainerTools:main Nov 25, 2024
11 checks passed
@Y--
Copy link
Contributor Author

Y-- commented Nov 25, 2024

Thanks a lot for all your help and moving the PR quickly @mattsanta !
Any idea when this will be released (or when is next release)? Thanks!!

@dntosas
Copy link

dntosas commented Dec 6, 2024

@mattsanta hey! are you aiming to release this anytime soon? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsanta mattsanta mattsanta approved these changes

Assignees

@mattsanta mattsanta

Labels

size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Y-- @mattsanta @dntosas