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

Integ test - creation/deletion of brupop and test pods improvement #217

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

gthao313
Copy link
Member

Issue number:
#216
#208

Description of changes:

Author: Tianhao Geng <tianhg@amazon.com>
Date:   Tue Jun 28 01:00:14 2022 +0000

    skip creating brupop resources if they already exist

    Integration test allows to launch multiple nodegroups on eks cluster,
    and it would be redundant to create brupop resources multiple times.
    Therefore this change make system to skip creating brupop resources
    if they already exist.

    Meanwhile, simplify the functionality of create and delete brupop
    resources.

commit 55587205f41633d823b3bef0ccaa91216f1b98cf
Author: Tianhao Geng <tianhg@amazon.com>
Date:   Tue Jun 28 00:33:44 2022 +0000

    skip creating test pods if they already exist

    Integration test allows to launch multiple nodegroups on eks cluster,
    and it would be redundant to create test pods multiple times. Therefore
    this change make system to skip creating test pods if they already
    exist.

Testing done:
Method: Launch multiple nodegroup and check if brupop resources and test pods are created only once (first nodegroup)

cargo run --bin integ integration-test --cluster-name brupop-ipv4 --region us-west-2 --bottlerocket-version 1.6.1  --arch x86_64 --nodegroup-name x86

cargo run --bin integ integration-test --cluster-name brupop-ipv4 --region us-west-2 --bottlerocket-version 1.6.1  --arch arm64 --nodegroup-name arm64

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@gthao313 gthao313 marked this pull request as ready for review June 28, 2022 01:24
@gthao313 gthao313 changed the title Integ test update improvement Integ test - creation and deletion of brupop and test pods improvement Jun 28, 2022
@gthao313 gthao313 changed the title Integ test - creation and deletion of brupop and test pods improvement Integ test - creation/deletion of brupop and test pods improvement Jun 28, 2022
integ/src/updater.rs Outdated Show resolved Hide resolved
//therefore, skips test pods creation if those pods already exist.
// This wouldn't affect pods deletion.
let error_content = String::from_utf8_lossy(&brupop_resource_output.stderr).to_string();
match error_content.contains("AlreadyExists") {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: IMO it's simpler to just use if instead of match on a boolean expression.

Integration test allows to launch multiple nodegroups on eks cluster,
and it would be redundant to create test pods multiple times. Therefore
this change make system to skip creating test pods if they already
exist.
Integration test allows to launch multiple nodegroups on eks cluster,
and it would be redundant to create brupop resources multiple times.
Therefore this change make system to skip creating brupop resources
if they already exist.

Meanwhile, simplify the functionality of create and delete brupop
resources.
@gthao313
Copy link
Member Author

Ideally, we have to clean up integration test when we need test a code change. However, during brupop release process, we need test arm and amd at same time (runcargo run --bin integ integration-test to add two nodegroups to cluster), and this will ask integration test to create brupop resources and test pods twice. Therefore, these process would fail on resources Already exist.

Solution: run kubectl apply -f xxx instead of kubectl create -f xxx.
apply intelligently skips the duplicate resources creation and only configure new or changed resources.

service/nginx unchanged
statefulset.apps/web-test configured
deployment.apps/nginx-test unchanged
poddisruptionbudget.policy/pod-disruption-budget-test unchanged
customresourcedefinition.apiextensions.k8s.io/bottlerocketshadows.brupop.bottlerocket.aws configured
namespace/brupop-bottlerocket-aws unchanged
clusterissuer.cert-manager.io/selfsigned-issuer unchanged
issuer.cert-manager.io/my-ca-issuer unchanged
certificate.cert-manager.io/brupop-apiserver unchanged
serviceaccount/brupop-apiserver-service-account unchanged
clusterrole.rbac.authorization.k8s.io/brupop-apiserver-role unchanged
clusterrolebinding.rbac.authorization.k8s.io/brupop-apiserver-role-binding unchanged
clusterrolebinding.rbac.authorization.k8s.io/brupop-apiserver-auth-delegator-role-binding unchanged
deployment.apps/brupop-apiserver configured
service/brupop-apiserver unchanged
serviceaccount/brupop-agent-service-account unchanged
clusterrole.rbac.authorization.k8s.io/brupop-agent-role unchanged
clusterrolebinding.rbac.authorization.k8s.io/brupop-agent-role-binding unchanged
daemonset.apps/brupop-agent configured
serviceaccount/brupop-controller-service-account unchanged
clusterrole.rbac.authorization.k8s.io/brupop-controller-role unchanged
clusterrolebinding.rbac.authorization.k8s.io/brupop-controller-role-binding unchanged
deployment.apps/brupop-controller-deployment unchanged
service/brupop-controller-server unchanged

Hence, more than once integration test at same time will not fail on resource alreadyexit, BUT we don't suggest to test multiple brupop code changes simultaneously.

@gthao313 gthao313 requested a review from cbgbt July 12, 2022 21:30
let brupop_resource_status = Command::new(KUBECTL_BINARY)
.args([
"apply",
&action_string.to_lowercase(),
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you plan to handle deletion failure? I know sometimes the deletion get stuck and need to manually patch the resource especially during the CRD update. Does it make sense to set up a timeout on deletion and print the manual patch command?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you share what situations you had met?

Copy link
Contributor

Choose a reason for hiding this comment

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

We had a discussion offline and decided to open an new issue if the above situation happens.

@gthao313 gthao313 merged commit d53fa93 into bottlerocket-os:develop Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants