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

Separating Upgrade test and ClickHouse Migration Tests #383

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

tushartathgur
Copy link
Contributor

@tushartathgur tushartathgur commented Jul 18, 2023

  • Latest changes in Antrea CRDs require us to delete the downgrade and check test, to keep it on par with Antrea, we have separated the upgrade test and the ClickHouse migration tests.
  • In order to test for the data schema management, we have added new ClickHouse migration test.

@codecov
Copy link

codecov bot commented Jul 18, 2023

Codecov Report

Merging #383 (f63c4f2) into main (c4aa6b0) will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #383      +/-   ##
==========================================
- Coverage   66.13%   66.07%   -0.06%     
==========================================
  Files          38       38              
  Lines        5049     5049              
==========================================
- Hits         3339     3336       -3     
- Misses       1553     1557       +4     
+ Partials      157      156       -1     
Flag Coverage Δ *Carryforward flag
python-coverage 56.37% <ø> (ø) Carriedforward from ed3fe31
unit-tests 69.66% <ø> (-0.09%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

see 2 files with indirect coverage changes

@tushartathgur tushartathgur force-pushed the upgrade_test branch 7 times, most recently from 88ae5f5 to 61605a7 Compare July 19, 2023 22:07
@tushartathgur tushartathgur changed the title Removing downgrade and check check from Theia Separating Upgrade test and ClickHouse Migration Tests Jul 19, 2023
Latest changes in Antrea CRDs require us to delete the downgrade and check test, to keep it on par with Antrea, we have separated the upgrade test and the ClickHouse migration tests.
In order to test for the data Schema management, we have added new ClickHouse migration test.

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtFLVDL.vmware.com>
@@ -249,7 +249,7 @@ sparkOperator:
tag: "v1beta2-1.3.3-3.1.1"
theiaManager:
# -- Determine whether to install Theia Manager.
enable: true
enable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

If you would like to exclude theia-manager from the ClickHouse upgrading test, I would suggest to add an option in generate manifest script for this. Maybe add an option clickhouse-only, so that we can exclude other components in case there are more in the future.
We would like to enable theia-manager by default as theia CLI has dependency on it and we would like to provide users this default functionality.

@@ -7282,7 +6965,7 @@ spec:
value: 1m
- name: SKIP_ROUNDS_NUM
value: "3"
image: projects.registry.vmware.com/antrea/theia-clickhouse-monitor:latest
image: projects.registry.vmware.com/antrea/antrea-ubuntu:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we have antrea image for clickhouse monitor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tbh I have no idea why it did that as it is a auto generated file and Values.yaml file was not changed for this to happen!

docker exec -i kind-control-plane dd of=/root/clickhouse-operator-install-bundle.yaml < build/charts/theia/crds/clickhouse-operator-install-bundle.yaml
./hack/generate-manifest.sh --mode release --local /data/clickhouse --no-grafana | docker exec -i kind-control-plane dd of=/root/flow-visibility-ch-only.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

As this is the upgrade test for the whole theia, maybe we can add other components like grafana, spark operator, and theia manager?

Comment on lines 232 to 237
pushd $TMP_DIR > /dev/null
export IMG_NAME=projects.registry.vmware.com/antrea/antrea-ubuntu
export IMG_TAG=$ANTREA_FROM_TAG
./hack/generate-manifest.sh --mode release | docker exec -i kind-control-plane dd of=/root/antrea.yml
popd
rm -rf $TMP_DIR
Copy link
Contributor

@yanjunz97 yanjunz97 Jul 20, 2023

Choose a reason for hiding this comment

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

Since we plan to use the same Antrea yaml file for the ClickHouse migration test, maybe we can remove all the contents related to ANTREA_FROM_TAG in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will require the ANTREA_FROM_TAG for installing the last release version of Theia for checking the upgrade from a release to the latest code. Similar arguments hold for THEIA_FROM_TAG ymls

@@ -0,0 +1,262 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not good at naming. But maybe test-migrate-clickhouse.sh will be better considering the consistency?

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
Copy link
Contributor

@yanjunz97 yanjunz97 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, let's merge it after the e2e tests pass

Signed-off-by: Tushar Tathgur <tathgurt@tathgurtPNQHP.vmware.com>
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.

2 participants