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

E2E usability updates #7353

Merged
merged 1 commit into from
Feb 7, 2024
Merged

Conversation

kaovilai
Copy link
Member

@kaovilai kaovilai commented Jan 24, 2024

  • Set GOBIN so Makefile don't modify $PATH on go install
  • Fix realPath resolving when cloud credentials is prefixed by ~ for home dir
  • Add Velero NS to get/delete call from CLI to ignore NS set in ~/.config/velero/config.json
  • Discover dockerconfig in home dir
  • Typo updates
  • e2e README.md updates

Signed-off-by: Tiger Kaovilai tkaovila@redhat.com

Thank you for contributing to Velero!

Please add a summary of your change

Does your change fix a particular issue?

Fixes #(issue)

Please indicate you've done the following:

  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Created a changelog file or added /kind changelog-not-required as a comment on this pull request.
  • Updated the corresponding documentation in site/content/docs/main.

Changes to make e2e tests work in my local env with kind cluster

make test-e2e CREDS_FILE=x BSL_BUCKET=x CLOUD_PROVIDER=kind OBJECT_STORE_PROVIDER=aws BSL_PREFIX=velero BSL_CONFIG="region=us-east-1" GINKGO_FOCUS="Basic\]\[ClusterResource";
Ran 3 of 54 Specs in 137.958 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 51 Skipped
PASS

@kaovilai kaovilai force-pushed the e2e-updates branch 3 times, most recently from b0e615e to 84c5dbd Compare January 24, 2024 19:53
@kaovilai kaovilai changed the title Set GOBIN so Makefile don't modify $PATH on go install E2E usability updates Jan 24, 2024
@kaovilai
Copy link
Member Author

/kind changelog-not-required

@github-actions github-actions bot added the kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes label Jan 25, 2024
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (99e0c48) 61.65% compared to head (b1d95cf) 61.65%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7353   +/-   ##
=======================================
  Coverage   61.65%   61.65%           
=======================================
  Files         262      262           
  Lines       28479    28479           
=======================================
  Hits        17558    17558           
  Misses       9689     9689           
  Partials     1232     1232           

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

@kaovilai kaovilai marked this pull request as ready for review January 25, 2024 01:31
test/util/velero/velero_utils.go Outdated Show resolved Hide resolved
test/util/velero/velero_utils.go Outdated Show resolved Hide resolved
@kaovilai
Copy link
Member Author

@danfengliu lmk if there is any problematic changes here. I can also split into smaller PRs if required.

@reasonerjt reasonerjt requested review from danfengliu and ywk253100 and removed request for reasonerjt and Lyndon-Li January 25, 2024 04:55
@danfengliu
Copy link
Contributor

@kaovilai I will run nightly against this PR, it seems no need to split it for now.

@danfengliu
Copy link
Contributor

danfengliu commented Jan 25, 2024

Hi @kaovilai , there're some cross parts with this PR 7292, so if this PR is merged first ( nightly CI has support IRSA, so I prefer to have PR 7292 go first, sorry for letting you wait for this), you might need to rebase main branch and solve some conflicts. And please help to review my PR, we're going to cover IRSA in nightly test.

@kaovilai
Copy link
Member Author

Sounds good

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe for another PR, but can't test/e2e/Makefile and test/perf/Makefile be merged to avoid duplication?

Copy link
Contributor

Choose a reason for hiding this comment

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

@qiuming-best , please help to provide your thought on this question, is there any block things from merging these 2 Makefile?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, maybe it could be merged into one, but may be more confusing to use.

I originally set the performance Makefile apart from the e2e Makefile for both tests, which all need lots of input parameters, and also both of them have some unique parameters. To not confuse I make them independent.

Comment on lines 70 to 77
focus:
# tests to focus on, use `|` to concatenate multiple regexes to run on the same job
- Basic\]\[ClusterResource
- Basic\]\[StorageClass
- PrivilegesMgmt\]\[SSR|ResourceModifier
- ResourceFiltering
- NamespaceMapping\]\[Single\]\[Restic|NamespaceMapping\]\[Multiple\]\[Restic
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding more coverage to the PR e2e

@danfengliu
Copy link
Contributor

Logic in line 78~88 should be moved to here, because this logic only triggered by function VeleroInstall , but most of tests will not install Velero if it’s not needed or Velero instance exists by previous test. so objectStoreProvider might not be set in these tests.

For example in backup deletion test, veleroCfg was set to the initial empty value here.

kaovilai added a commit to kaovilai/velero that referenced this pull request Feb 2, 2024
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai kaovilai requested a review from danfengliu February 2, 2024 22:07
args = append(args, "--features", options.Features)
if strings.EqualFold(options.Features, FeatureCSI) && options.UseVolumeSnapshots {
if strings.EqualFold(cloudProvider, "azure") {
if strings.EqualFold(cloudProvider, "azure") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if strings.EqualFold(cloudProvider, "azure") {
if len(options.Features) > 0 {
args = append(args, "--features", options.Features)
if strings.EqualFold(options.Features, FeatureCSI) && options.UseVolumeSnapshots &&
strings.EqualFold(cloudProvider, "azure") {
fmt.Println("Start to install Azure VolumeSnapshotClass ...")
if err := KubectlApplyByFile(ctx, "../util/csi/AzureVolumeSnapshotClass.yaml"); err != nil {
return err

If options.Features is not empty, arg --features must be set with that value no matter what provider it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current code blocked data mover pipeline due to args --features is missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@danfengliu
Copy link
Contributor

Logic in line 78~88 should be moved to here, because this logic only triggered by function VeleroInstall , but most of tests will not install Velero if it’s not needed or Velero instance exists by previous test. so objectStoreProvider might not be set in these tests.
For example in backup deletion test, veleroCfg was set to the initial empty value here.

Sorry I also made a mistake, after re-running nightly against your latest code, test failed due to missing --provider , this is cause by moving line 78~88 to another location, this logic should not be deleted from the original place, because it is used when isStandbyCluster is true, please recover line 78~88, which means we have this logic in 2 places, you can consider wrap it to a function.

kaovilai added a commit to kaovilai/velero that referenced this pull request Feb 5, 2024
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
…th resolving when cloud credentials is prefixed by `~` for home dir Use `~/.docker/config.json` if REGISTRY_CREDENTIAL_FILE not defined and skip step if does not exists since it is optional

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Set `GOBIN` so Makefile don't modify $PATH on `go install`
Fix realPath resolving when cloud credentials is prefixed by `~` for home dir
Use `~/.docker/config.json` if REGISTRY_CREDENTIAL_FILE not defined and skip step if does not exists since it is optional

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add kind testdata storageclass

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add kind testdata storageclass

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

log `Start to install Azure VolumeSnapshotClass ...` only on azure when csi is enabled

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add BSL_CONFIG example and notes

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Makefile: Set `GOBIN` for `_output/...`

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

README spacing

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

StandbyClusterObjectStoreProvider typo

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Specify velero namespace during get/delete command

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Use object stores rather than cloudProvider for bucket queries

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Remove debug print

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

simplify NS get changes, add velero NS to `DeleteBackupResource`

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Skip file system backups on kind which uses hostPath volumes

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add StorageClass change test to PR kind e2e

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add more tests to pr

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add NS mapping to PR e2e

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Add `SKIP_KIND` to some jobs containing volumes

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Remove kind from kibishii tests

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Label volume resource policies as restic, skip restic/snapshot tests, add more tests

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

TTLTest is a snapshot test

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Remove non working tests

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Resolve vmware-tanzu#7353 (comment)

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

address https://github.com/vmware-tanzu/velero/pull/7353/files#r1477218762

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>

Address vmware-tanzu#7353 (comment)

Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
Copy link
Contributor

@danfengliu danfengliu left a comment

Choose a reason for hiding this comment

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

LGTM

@blackpiglet blackpiglet merged commit 3b8370e into vmware-tanzu:main Feb 7, 2024
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has-e2e-tests kind/changelog-not-required PR does not require a user changelog. Often for docs, website, or build changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants