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

Copy "envFrom" from Velero server when creating maintenance jobs #8343

Merged
merged 2 commits into from
Nov 18, 2024

Conversation

evhan
Copy link

@evhan evhan commented Oct 23, 2024

Thank you for contributing to Velero!

Please add a summary of your change

When the Velero server creates a maintenance job, it copies its own env into the newly-created Pod spec. However, Velero's environment may be populated via envFrom in addition to env, in which case the maintenance job may fail since it will be missing potentially-important variables.

We ran into this while using the OpenStack plugin, but the issue is not specific to that provider. We worked around the problem in the meantime by moving all variables that were previously stored in a Secret into the Deployment itself.

The documentation already says:

Maintenance jobs will inherit the labels, annotations, toleration, nodeSelector, service account, image, environment variables, cloud-credentials etc. from Velero deployment.

This would appear to be covered by that, and the change is just bringing the implementation in line with the docs.

Does your change fix a particular issue?

No existing issue in GitHub from what I can see.

Please indicate you've done the following:

Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 58.98%. Comparing base (ebbeb7a) to head (f981dd4).
Report is 46 commits behind head on main.

Files with missing lines Patch % Lines
pkg/util/velero/velero.go 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8343      +/-   ##
==========================================
- Coverage   58.99%   58.98%   -0.01%     
==========================================
  Files         367      367              
  Lines       38847    38889      +42     
==========================================
+ Hits        22918    22940      +22     
- Misses      14467    14487      +20     
  Partials     1462     1462              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

blackpiglet
blackpiglet previously approved these changes Oct 24, 2024
@Lyndon-Li
Copy link
Contributor

This looks like a valid issue for both maintenance job and data mover pods, but by default Velero doesn't set envFrom, so we will not include it in 1.15 RC.2.

@Lyndon-Li
Copy link
Contributor

@evhan
Could you also help to fix the same problem for data mover pods? The code should be around functions createBackupPod and createRestorePod.

Copy link
Contributor

@reasonerjt reasonerjt left a comment

Choose a reason for hiding this comment

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

@evhan Thanks for the PR. We appreciate your contribution.

But could you also add a UT?

@evhan
Copy link
Author

evhan commented Oct 28, 2024

@evhan Thanks for the PR. We appreciate your contribution.

But could you also add a UT?

Sure. I'll also look at fixing the issue for the data mover pods. Would you like that in the same PR or a new one?

@Lyndon-Li
Copy link
Contributor

Would you like that in the same PR or a new one?

All been included in the current PR will be fine. Thanks.

@evhan evhan dismissed stale reviews from shubham-pampattiwar and blackpiglet via 5a53dbd October 30, 2024 00:36
@evhan evhan force-pushed the maintenance-job-env-from branch from 59eda97 to 5a53dbd Compare October 30, 2024 00:36
@evhan
Copy link
Author

evhan commented Oct 30, 2024

All been included in the current PR will be fine. Thanks.

No problem. I've added the same treatment for the data mover pods and added env/envFrom to the tests to make sure they're included. Hope that looks OK.

Copy link
Collaborator

@shubham-pampattiwar shubham-pampattiwar left a comment

Choose a reason for hiding this comment

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

Thank you @evhan for the contribution. CI is failing on make update. Can please you execute make update and update the pull request ?

@evhan evhan force-pushed the maintenance-job-env-from branch from 5a53dbd to 293b730 Compare October 30, 2024 02:02
@evhan
Copy link
Author

evhan commented Oct 30, 2024

Thank you @evhan for the contribution. CI is failing on make update. Can please you execute make update and update the pull request ?

Yeah, just noticed, sorry about that. Should be fixed now!

Copy link
Member

@kaovilai kaovilai left a comment

Choose a reason for hiding this comment

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

This LGTM. Thank you!

@reasonerjt reasonerjt merged commit e7da672 into vmware-tanzu:main Nov 18, 2024
44 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants