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

Fix issues in e2e test #442

Merged
merged 16 commits into from
Mar 12, 2024
Merged

Fix issues in e2e test #442

merged 16 commits into from
Mar 12, 2024

Conversation

kevinliu24
Copy link
Contributor

@kevinliu24 kevinliu24 commented Feb 16, 2024

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@kevinliu24 kevinliu24 added do not review Progress: not ready for the code review yet type/enhancement Type: make the code neat or more efficient wip Progress: work in progress labels Feb 16, 2024
@kevinliu24 kevinliu24 self-assigned this Feb 16, 2024
@@ -183,8 +183,7 @@ func ObjectEqual(newUnstruct, oldUnstruct *unstructured.Unstructured) bool {
annotations := map[string]string{}
for k, v := range oldUnstruct.GetAnnotations() {
if k == annotation.AnnLastAppliedConfigKey ||
k == annotation.AnnRestartTimestamp ||
k == annotation.AnnLastReplicas {
k == annotation.AnnRestartTimestamp {
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove this annotation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MegaByte875 I removed this check to fix the Graphd is stuck in "Scale In" though the nebula cluster is ready issue. This is due to new_replicas not equal to last_replicas which is because last_replicas wasn't updated to the correct value. So we shouldn't ignore the annotation AnnLastReplicas when checking equality so we can perform an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to @MegaByte875 k == annotation.AnnLastReplicas was added to fix a bug. Will leave alone for know. @MegaByte875 will look into why the replicas are not equal.

tests/e2e/envfuncsext/nebulacluster-ready-func.go Outdated Show resolved Hide resolved
tests/e2e/config/config.go Outdated Show resolved Hide resolved
tests/e2e/envfuncsext/nebulacluster.go Outdated Show resolved Hide resolved
tests/e2e/main_test.go Outdated Show resolved Hide resolved
tests/e2e/main_test.go Outdated Show resolved Hide resolved
tests/e2e/nebulacluster_test.go Outdated Show resolved Hide resolved
tests/e2e/nebulacluster_test.go Show resolved Hide resolved
@kevinliu24 kevinliu24 marked this pull request as ready for review March 7, 2024 00:07
@kevinliu24 kevinliu24 removed the do not review Progress: not ready for the code review yet label Mar 7, 2024
@@ -57,9 +57,16 @@ jobs:
export E2E_DOCKER_CONFIG_JSON_SECRET=`cat ~/.docker/config.json| base64 -w 0`
make e2e E2EARGS="-v=5 -skip-features 'pv expansion|custom config for dynamic|tools for exporter'"
env:
E2E_AWS_ACCESS_KEY_ID: ${{ secrets.AWS_ACCESS_KEY_ID }}
E2E_AWS_SECRET_ACCESS_KEY: ${{ secrets.AWS_SECRET_ACCESS_KEY }}
E2E_BR_IMAGE: reg.vesoft-inc.com/cloud-dev/br-ent
Copy link
Contributor

Choose a reason for hiding this comment

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

Why put it in cloud-dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently br-ent is in cloud-dev. @MegaByte875 to move them into the CI folder

E2E_OPERATOR_IMAGE: reg.vesoft-inc.com/ci/nebula-operator:ci-e2e
E2E_OPERATOR_INSTALL: "true"
E2E_NC_VERSION: v3.6.0
E2E_NC_VERSION: v3.7.0
E2E_NC_AGENT_IMAGE: reg.vesoft-inc.com/cloud-dev/nebula-agent
Copy link
Contributor

Choose a reason for hiding this comment

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

Why put it in cloud-dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MegaByte875 to move them into the CI folder

tests/e2e/envfuncsext/nebula-command.go Outdated Show resolved Hide resolved
tests/e2e/envfuncsext/nebulacluster-ready-func.go Outdated Show resolved Hide resolved
tests/e2e/envfuncsext/nebulacluster.go Outdated Show resolved Hide resolved
pkg/kube/cronbackup.go Outdated Show resolved Hide resolved
Copy link
Contributor

@MegaByte875 MegaByte875 left a comment

Choose a reason for hiding this comment

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

LGTM

tests/e2e/envfuncsext/backup.go Outdated Show resolved Hide resolved
@MegaByte875 MegaByte875 merged commit 42b0812 into master Mar 12, 2024
2 of 3 checks passed
@kevinliu24 kevinliu24 deleted the fix_e2e_tests branch March 12, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Type: make the code neat or more efficient wip Progress: work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants