feat(backend): Remove PipelineSpec Template storage from ObjStore responsibilies. Fixes #10509 #10790
Conversation
|
/test kubeflow-pipeline-backend-test |
gmfrasca
force-pushed
the
rm-objstore-pipeline
branch
from
84ebbf7
to
32c5462
Compare
|
@gmfrasca: The following tests failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
…ponsibilies. Fixes kubeflow#10509 Signed-off-by: Giulio Frasca <gfrasca@redhat.com>
gmfrasca
force-pushed
the
rm-objstore-pipeline
branch
from
32c5462
to
88fbab6
Compare
Signed-off-by: Giulio Frasca <gfrasca@redhat.com>
gmfrasca
force-pushed
the
rm-objstore-pipeline
branch
from
88fbab6
to
ee7866b
Compare
|
|
||
| // Switch to a bad object store | ||
| manager.objectStore = &FakeBadObjectStore{} | ||
|
|
||
| // Delete the above pipeline_version. | ||
| err = manager.DeletePipelineVersion(FakeUUIDOne) | ||
| assert.NotNil(t, err) | ||
|
|
||
| // Verify the version in deleting status. | ||
| version, err := manager.pipelineStore.GetPipelineVersionWithStatus(FakeUUIDOne, model.PipelineVersionDeleting) | ||
| assert.Nil(t, err) | ||
| assert.NotNil(t, version) |
There was a problem hiding this comment.
it didn't jump out at me why this entire block is being deleted. Is there nothing here test-worthy if the object store storage goes away? Specifically wondering if it makes sense to keep the check of the deleting status.
There was a problem hiding this comment.
yeah, this specific test can probably remain, albeit the expected results are now the exact opposite: the RM should not throw an error if the ObjStore is bad, should be able to delete the pipelineversion, and the pipelineversion should not exist after performing the delete operation (essentially flip the asserts in L1533, L1537, and L1538)
| PipelineId: DefaultFakeUUID, | ||
| Name: "pipeline_version", | ||
| PipelineId: DefaultFakeUUID, | ||
| PipelineSpec: "apiVersion: argoproj.io/v1alpha1\nkind: Workflow", |
There was a problem hiding this comment.
how is this related to this PR?
There was a problem hiding this comment.
If a mock PipelineSpec isn't provided, the Mock ResourceManager will attempt to fetch the spec from ObjectStore which will break as it no longer is uploaded there. This behavior only exists in the test RM, I believe
There was a problem hiding this comment.
yep:
this is now fetched from the PipelineSpec column in the DB.…k ObjStore - Add PipelineSpec to mock PVs as they are no longer retrieved from ObjStore Signed-off-by: Giulio Frasca <gfrasca@redhat.com>
gmfrasca
force-pushed
the
rm-objstore-pipeline
branch
from
ee7866b
to
4bbea81
Compare
|
what's the status of this? Just need reviews? |
yes, believe so |
|
Confirmed this with pre-existing pipelines, they continue to be fetched successfully. New pipelines as well, and they are not stored in the object store. Great work! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: HumairAK 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 |
Description of your changes:
Fixes #10509
The PipelineSpec definition is currently stored in two places, DB and ObjStore, creating the potential for a competing source-of-truth problem. The ObjStore copy doesn't appear to actually be used/retrieved anywhere, so remove it from the list of responsibilities
Checklist: