Skip to content

server: fix panic when default annotations are specified #8867

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
merged 1 commit into from
Jan 3, 2025
Merged

server: fix panic when default annotations are specified #8867

merged 1 commit into from
Jan 3, 2025

Conversation

@haircommander
Copy link
Member

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fix a panic when default_annotations are used

@openshift-ci openshift-ci bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 20, 2024
@openshift-ci openshift-ci bot requested review from QiWang19 and hasan4791 December 20, 2024 18:21
@openshift-ci openshift-ci bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Dec 20, 2024
@haircommander
Copy link
Member Author

/kind bug

@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 20, 2024
@haircommander haircommander force-pushed the default_annotations-fix branch from 129a42b to 0fe2188 Compare December 20, 2024 18:26
kannon92
kannon92 reviewed Dec 20, 2024
View reviewed changes
Copy link
Contributor

@kannon92 kannon92 left a comment

Choose a reason for hiding this comment

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

Can we add a unit test to replicate the panic and make sure it is fixed?

@haircommander haircommander force-pushed the default_annotations-fix branch from 0fe2188 to 4093c1b Compare December 20, 2024 18:33
@haircommander
Copy link
Member Author

I have yet to reproduce the panic in integration tests unfortunately. I feel like it's worth getting a change out as saving the map in the sandbox and then editing it later would cause this, but I suspect it'll be tricky to hit the exact race we do in prod locally

@kannon92
Copy link
Contributor

I have yet to reproduce the panic in integration tests unfortunately. I feel like it's worth getting a change out as saving the map in the sandbox and then editing it later would cause this, but I suspect it'll be tricky to hit the exact race we do in prod locally

I still think a unit test would be sufficient to catch this. We have mocks for a reason.

@codecov
Copy link

codecov bot commented Dec 20, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 47.08%. Comparing base (b7f3c24) to head (997e4fb).
Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8867      +/-   ##
==========================================
- Coverage   47.09%   47.08%   -0.02%     
==========================================
  Files         154      154              
  Lines       22136    22138       +2     
==========================================
- Hits        10426    10424       -2     
- Misses      10642    10645       +3     
- Partials     1068     1069       +1

@haircommander
Copy link
Member Author

I still think a unit test would be sufficient to catch this. We have mocks for a reason.

the mocks don't get to this point--we're not fully unit testing the sandbox creation process. That's part of the motivation of #8289

That said, I did reproduce with an integration test, and verify my fix seems to fix. I am trying to find a reasonable number of containers to create. It always seems to reproduce at 1000 and I think I caught it once at 100, but even that feels like a bit much so I'm inclined to not include a test

@kannon92
Copy link
Contributor

Given the revert it would be nice to have some kind of test (unit / integration / e2e) that would cover this. I guess a e2e test with that many containers may be out of the realm for origin?

hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Dec 21, 2024
@hswong3i
alvistack/v1.32.0
b2e8a7c 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Dec 23, 2024
@hswong3i
alvistack/v1.32.0
b5adafd 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@roman-kiselenko roman-kiselenko mentioned this pull request Dec 24, 2024
Closed
@sohankunkerkar
Copy link
Member

/retest

1 similar comment
@haircommander
Copy link
Member Author

/retest

@haircommander haircommander mentioned this pull request Jan 2, 2025
Merged
@sohankunkerkar
Copy link
Member

/retest

@haircommander haircommander force-pushed the default_annotations-fix branch from 4093c1b to 7eaad95 Compare January 2, 2025 21:55
@haircommander
Copy link
Member Author

okay I figured out a test. it doesn't catch the panic, but does catch the context for the panic (that we didn't deep copy a map).

@cri-o/cri-o-maintainers this should be ready PTAL

@haircommander haircommander force-pushed the default_annotations-fix branch from 7eaad95 to ce08447 Compare January 2, 2025 21:56
@haircommander haircommander force-pushed the default_annotations-fix branch from ce08447 to 997e4fb Compare January 2, 2025 22:23
hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Jan 3, 2025
@hswong3i
alvistack/v1.32.0
0ba5b79 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Jan 3, 2025
@hswong3i
alvistack/v1.32.0
9b0afe2 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
@haircommander
Copy link
Member Author

/retest
quay flakes

sohankunkerkar
sohankunkerkar approved these changes Jan 3, 2025
View reviewed changes
Copy link
Member

@sohankunkerkar sohankunkerkar left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, sohankunkerkar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [haircommander,sohankunkerkar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sohankunkerkar
Copy link
Member

In fact, pull progress timeout should not timeout when set to 0 is a bit flaky too. We have a tracking issue for that: #8872

@openshift-merge-bot openshift-merge-bot bot merged commit cbb9d83 into cri-o:main Jan 3, 2025
63 of 70 checks passed
@sohankunkerkar
Copy link
Member

/cherry-pick release-1.31

@openshift-cherrypick-robot
Copy link

@sohankunkerkar: new pull request created: #8885

In response to this:

/cherry-pick release-1.31

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 3, 2025
Merged
@sohankunkerkar
Copy link
Member

/cherry-pick release-1.32

@openshift-cherrypick-robot
Copy link

@sohankunkerkar: new pull request created: #8886

In response to this:

/cherry-pick release-1.32

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Jan 3, 2025
Merged
hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Jan 11, 2025
@hswong3i
alvistack/v1.32.0
a0e16fb 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868
See cri-o#8902
See cri-o#8903

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
hswong3i added a commit to alvistack/ansible-role-cri_o that referenced this pull request Jan 11, 2025
@hswong3i
https://github.com/alvistack/cri-o-cri-o/commit/a0e16fbeb3a9cee4dce37…
7d3b998 
…a191585accdedc6e382

See cri-o/cri-o#8867
See cri-o/cri-o#8868
See cri-o/cri-o#8902
See cri-o/cri-o#8903

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
hswong3i added a commit to alvistack/cri-o-cri-o that referenced this pull request Jan 18, 2025
@hswong3i
alvistack/v1.32.0
9a9ba85 
    git clean -xdf
    go mod download
    go mod vendor
    tar zcvf ../cri-o_1.32.0.orig.tar.gz --exclude=.git .
    debuild -uc -us
    cp cri-o.spec ../cri-o_1.32.0-1.spec
    cp ../cri-o*1.32.0*.{gz,xz,spec,dsc} /osc/home\:alvistack/cri-o-cri-o-1.32.0/
    rm -rf ../cri-o*1.32.0*.*

See cri-o#8867
See cri-o#8868
See cri-o#8902
See cri-o#8903

Signed-off-by: Wong Hoi Sing Edison <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sohankunkerkar sohankunkerkar sohankunkerkar approved these changes

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

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@QiWang19 QiWang19 Awaiting requested review from QiWang19

+1 more reviewer

@kannon92 kannon92 kannon92 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@sohankunkerkar sohankunkerkar

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@haircommander @kannon92 @sohankunkerkar @openshift-cherrypick-robot