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

[Testing] KFP standalone test infra for upgradability #1971

Merged
merged 30 commits into from
Mar 9, 2020

Conversation

Bobgy
Copy link
Contributor

@Bobgy Bobgy commented Aug 28, 2019

Fixes #1638
/area testing


This change is Reviewable

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 28, 2019

Verified locally, the process of deploy -> upgrade works. I need to add some integration tests to verify resources.

@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 29, 2019

/retest

1 similar comment
@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 29, 2019

/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Sep 3, 2019

/assign @IronPan

Not fully finished on upgrade_test.go, but you can first review overall structure of the upgrade test.

@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 8, 2019

@IronPan
Copy link
Member

IronPan commented Nov 13, 2019

Mostly LGTM. Thanks Yuan. Could you check why job.trigger is inconsistent

@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 14, 2019

/retest
to verify test flakiness

@Bobgy
Copy link
Contributor Author

Bobgy commented Nov 14, 2019

@IronPan thanks, I will verify that

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 9, 2020

@IronPan
The difference problem I mentioned before was

Diff:
            	--- Expected
            	+++ Actual
            	@@ -30,3 +30,7 @@
            	   CronSchedule: (*job_model.APICronSchedule)(<nil>),
            	-  PeriodicSchedule: (*job_model.APIPeriodicSchedule)(<nil>)
            	+  PeriodicSchedule: (*job_model.APIPeriodicSchedule)({
            	+   EndTime: (strfmt.DateTime) 0001-01-01T00:00:00.000Z,
            	+   IntervalSecond: (int64) 0,
            	+   StartTime: (strfmt.DateTime) 0001-01-01T00:00:00.000Z
            	+  })
            	  }),
Test:       	TestUpgrade/TestVerify

I think it doesn't affect behavior, might be caused by some inconsistency of writing to and reading from db model.

UPDATE: I found the culprit: https://github.com/Bobgy/pipelines/blob/bec3c94a4505de4ff9129a8e598ef9aa2d6dff17/backend/src/apiserver/storage/job_store.go#L358
When updating a Job from persistence agent, it defaults interval value to 0. This is fine, cron string is also defaulted to empty string.
However, when converting model to api: https://github.com/Bobgy/pipelines/blob/bec3c94a4505de4ff9129a8e598ef9aa2d6dff17/backend/src/apiserver/server/api_converter.go#L305, cron string checks for empty string, but interval doesn't check for 0.

I will fix it in this PR.
This problem isn't triggered in original integration tests, because creating and fetching the job was too fast. Persistence agent didn't get a chance to update Job from swf CR. However, with upgrade tests, there are long enough time for persistence agent to update.

@Bobgy Bobgy changed the title Test KFP standalone deployment upgradability [Testing] KFP standalone deployment upgradability Mar 9, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 9, 2020

/retest

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 9, 2020

/test kubeflow-pipeline-upgrade-test

@Bobgy Bobgy changed the title [Testing] KFP standalone deployment upgradability [Testing] KFP standalone upgradability test infra Mar 9, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 9, 2020

/test kubeflow-pipeline-upgrade-test
strange, error messages says the test was interrupted, I'm guessing that's a prow issue because this job doesn't have enough priority

@Bobgy Bobgy changed the title [Testing] KFP standalone upgradability test infra [Testing] KFP standalone test infra for upgradability Mar 9, 2020
@IronPan
Copy link
Member

IronPan commented Mar 9, 2020

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: IronPan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 9, 2020

/retest

@k8s-ci-robot k8s-ci-robot merged commit 5391e88 into kubeflow:master Mar 9, 2020
@Bobgy Bobgy deleted the upgrade_tests branch March 10, 2020 01:46
Jeffwan pushed a commit to Jeffwan/pipelines that referenced this pull request Dec 9, 2020
* Implement upgrade test

* mark upgrade-tests.sh as executable

* Fix comments

* Base upgrade_test_setup.yaml

* e2e integration of upgrade test

* Fix entrypoint argument

* Fix e2e workflow yaml

* Fix run_test.sh argument processing

* Fix no closing backtick

* Restrucutre upgrade_test.go to focus the test on upgrade verification

* clean up code

* Clean up after upgrade test when it is run in integration tests.

* Include pipeline tests in upgrade test

* Reorder tests

* Add upgrade test coverage for run api resources

* Add job api resource coverage in upgrade test & refactored upgrade test

* Fix add missing step in upgrade test

* Fix BUILD.bazel

* Fix upgrade_test.go

* Try to fix upgrade test failure

* Fix hard coded namespace

* Sync upgrade-tests.sh with new changes in presubmit-tests-with-pipeline-deployment.sh

* Update upgrade test

* Remove redundant code

* Fix integration test exit code

* Fix trigger interval second mismatch
magdalenakuhn17 pushed a commit to magdalenakuhn17/pipelines that referenced this pull request Oct 22, 2023
Signed-off-by: Zhongcheng Lao <Zhongcheng.Lao@microsoft.com>
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.

Upgradability for Kubeflow Pipeline
3 participants