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.

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

fix: avoid to check backup container ID if pod container statuses are missing #4765

Merged
merged 11 commits into from
Jun 4, 2024
Merged

fix: avoid to check backup container ID if pod container statuses are missing #4765

merged 11 commits into from
Jun 4, 2024

Conversation

gabriele-wolfox
Copy link
Contributor

@gabriele-wolfox gabriele-wolfox commented Jun 3, 2024 •
edited by jsilvela
Loading

Closes #4767

@gabriele-wolfox gabriele-wolfox requested a review from a team as a code owner June 3, 2024 12:47
@github-actions github-actions bot added backport-requested ◀️ This pull request should be backported to all supported releases release-1.21 release-1.22 release-1.23 labels Jun 3, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 3, 2024

❗ By default, the pull request is configured to backport to all release branches.

  • To stop backporting this pr, remove the label: backport-requested ◀️ or add the label 'do not backport'
  • To stop backporting this pr to a certain release branch, remove the specific branch label: release-x.y

jsilvela
jsilvela requested changes Jun 3, 2024
View reviewed changes
Copy link
Collaborator

@jsilvela jsilvela left a comment

Choose a reason for hiding this comment

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

I can see other occurrences of ContainerStatuses[0], worth fixing while you're here.
And perhaps check for other [0]...

@gabriele-wolfox
Copy link
Contributor Author

I can see other occurrences of ContainerStatuses[0], worth fixing while you're here. And perhaps check for other [0]...

I put the focus on the error in the log, but It makes sense to fix them too.

mnencia
mnencia reviewed Jun 4, 2024
View reviewed changes
tests/e2e/upgrade_test.go Outdated Show resolved Hide resolved
mnencia
mnencia reviewed Jun 4, 2024
View reviewed changes
api/v1/backup_types.go Outdated Show resolved Hide resolved
@gabriele-wolfox gabriele-wolfox force-pushed the dev/4960 branch 2 times, most recently from 60f44c7 to 7ef24f9 Compare June 4, 2024 09:23
@jsilvela jsilvela self-requested a review June 4, 2024 10:18
@jsilvela
Copy link
Collaborator

jsilvela commented Jun 4, 2024

/test feature_type=backup-restore test_level=4

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 4, 2024

@jsilvela, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/9365377472

@github-actions github-actions bot added the ok to merge 👌 This PR can be merged label Jun 4, 2024
@jsilvela jsilvela removed the ok to merge 👌 This PR can be merged label Jun 4, 2024
@jsilvela
Copy link
Collaborator

jsilvela commented Jun 4, 2024

manually took out the ok-to-merge flag since I only tested the backup tests in the slash-test

leonardoce
leonardoce requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@leonardoce leonardoce left a comment

Choose a reason for hiding this comment

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

While this patch is fixing the underlying issue, we could easily prevent this specific bug from happening in the future, too.

We could change the signature of SetAsStarted to accept the pod name and the container ID, instead of accepting a pointer to the whole Pod. That would force us to actually check what is really happening.

internal/controller/backup_controller.go Outdated Show resolved Hide resolved
internal/controller/backup_controller.go Show resolved Hide resolved
@gabriele-wolfox @leonardoce
fix: avoid to check backup container ID if pod container statuses are…
4706732 
… missing

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
@gabriele-wolfox @leonardoce
fix: add same check when getting container ID
805448f 
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
gabriele-wolfox and others added 7 commits June 4, 2024 16:49
@gabriele-wolfox @leonardoce
fix: robustify e2e and check in backup controller
0087efa
@gabriele-wolfox @leonardoce
fix: check container status before setting a backup as started
2bd7028
@gabriele-wolfox @leonardoce
fix: check container status before setting a backup as started
0d1aed9
@jsilvela @leonardoce
chore: review, add retry logic
f602735 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
chore: apply gabriele-wolfox feedback
7f2beb1 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@jsilvela @leonardoce
fix: mis-applied gabriele-wolfox feedback
76e9404 
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
@gabriele-wolfox @leonardoce
fix: address Leonardo's comments
131ce73 
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
@leonardoce
Update internal/controller/backup_controller.go
00772ff 
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
@leonardoce leonardoce self-assigned this Jun 4, 2024
@leonardoce
Copy link
Contributor

E2e tests: https://github.com/EnterpriseDB/cloudnative-pg/actions/runs/9369519768

@gabriele-wolfox
fix: linter issue
c44793d 
Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
@leonardoce
Copy link
Contributor

/ok-to-merge Amazing!!! They're all green!!!

@cnpg-bot cnpg-bot added the ok to merge 👌 This PR can be merged label Jun 4, 2024
@leonardoce leonardoce merged commit 9515ee7 into cloudnative-pg:main Jun 4, 2024
24 of 25 checks passed
cnpg-bot pushed a commit that referenced this pull request Jun 4, 2024
@gabriele-wolfox
fix: panic in backup controller when pod container statuses are missi…
15ee5da 
…ng (#4765)

Closes #4767

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
Co-authored-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
(cherry picked from commit 9515ee7)
@github-actions github-actions bot mentioned this pull request Jun 4, 2024
Closed
leonardoce pushed a commit that referenced this pull request Jun 4, 2024
@gabriele-wolfox @leonardoce
fix: panic in backup controller when pod container statuses are missi…
eb960d6 
…ng (#4765)

Closes #4767

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
Co-authored-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
(cherry picked from commit 9515ee7)
leonardoce pushed a commit that referenced this pull request Jun 4, 2024
@gabriele-wolfox @leonardoce
fix: panic in backup controller when pod container statuses are missi…
6e6a7e2 
…ng (#4765)

Closes #4767

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
Co-authored-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
(cherry picked from commit 9515ee7)
dougkirkley pushed a commit to dougkirkley/cloudnative-pg that referenced this pull request Jun 11, 2024
@gabriele-wolfox @jsilvela @leonardoce
fix: panic in backup controller when pod container statuses are missi…
c599b82 
…ng (cloudnative-pg#4765)

Closes cloudnative-pg#4767

Signed-off-by: Gabriele Quaresima <gabriele.quaresima@enterprisedb.com>
Signed-off-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Signed-off-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
Co-authored-by: Jaime Silvela <jaime.silvela@enterprisedb.com>
Co-authored-by: Leonardo Cecchi <leonardo.cecchi@gmail.com>
Signed-off-by: Douglass Kirkley <dkirkley@eitccorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnencia mnencia mnencia left review comments

@jsilvela jsilvela jsilvela approved these changes

@leonardoce leonardoce leonardoce approved these changes

@NiccoloFei NiccoloFei Awaiting requested review from NiccoloFei NiccoloFei is a code owner

@litaocdl litaocdl Awaiting requested review from litaocdl litaocdl is a code owner

Assignees

@leonardoce leonardoce

Labels
backport-requested ◀️ This pull request should be backported to all supported releases ok to merge 👌 This PR can be merged release-1.21 release-1.22 release-1.23
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Panic while checking backup container ID if pod container statuses are missing
5 participants
@gabriele-wolfox @jsilvela @leonardoce @mnencia @cnpg-bot