8000 ⚠️ Change FybrikStorageAccount CRD by shlomitk1 · Pull Request #1855 · fybrik/fybrik · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

⚠️ Change FybrikStorageAccount CRD #1855

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Jan 12, 2023
Merged

⚠️ Change FybrikStorageAccount CRD #1855

merged 21 commits into from
Jan 12, 2023

Conversation

shlomitk1
Copy link
Contributor
@shlomitk1 shlomitk1 commented Jan 1, 2023 •
edited
Loading

Closes #1744
FybrikStorageAccount structure has been changed:

  • Added Type
  • Renamed Region -> Geography
  • Removed Endpoint
  • Added additional properties as per connection
  • Changed version to v1beta2

Structures are defined in pkg/storage/apis.

Example of the spec:

spec:
  id: theshire-object-store
  type: s3
  secretRef: credentials-theshire
  geography: theshire
  s3:
    region: eu
    endpoint: http://s3.eu.cloud-object-storage.appdomain.cloud

@shlomitk1 shlomitk1 marked this pull request as ready for review January 3, 2023 13:49
@shlomitk1
fix auto-generated files
c6c2590 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
revert last change
0c8ec80 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1 shlomitk1 changed the title ⚠️ [WIP] Change FybrikStorageAccount ⚠️ Change FybrikStorageAccount CRD Jan 3, 2023
@shlomitk1 shlomitk1 mentioned this pull request Jan 4, 2023
Closed
@roytman
Copy link
Collaborator
roytman commented Jan 4, 2023

Meantime, I have several small style related comments, and need to read the code more.

However, this PR breaks backward compatibility and requires uninstallation of an old Fybrik deployment and installation of a new one. Another option is to leave the old v1beta1.FybrikStorageAccount CRD, add the v1beta2.FybrikStorageAccount CRD, and provide a script/tool to transfer instances of v1beta1 to v1beta2.

@shlomitk1
leave v1beta1 + conversion
0fea303 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
Copy link
Contributor Author

Meantime, I have several small style related comments, and need to read the code more.

However, this PR breaks backward compatibility and requires uninstallation of an old Fybrik deployment and installation of a new one. Another option is to leave the old v1beta1.FybrikStorageAccount CRD, add the v1beta2.FybrikStorageAccount CRD, and provide a script/tool to transfer instances of v1beta1 to v1beta2.

OK. Leaving v1beta1 along with v1beta2. UnmarshalJSON takes care of the conversion to the expected structure.

@shlomitk1
add missing update
372745a 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
Merge branch 'master' into sa-crd
1a14c6c
simanadler
simanadler previously requested changes Jan 8, 2023
View reviewed changes
Copy link
Member
@simanadler simanadler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

As @roytman pointed out, this PR has backward compatibility implications. 1) It should be marked as such 2) The storage CRDs in the quick start examples need to be updated, as does the kfp sample 3) Does this affect any of the OPA policies in any of our examples?

pkg/storage/apis/app/v1beta2/fybrikstorageaccount_types.go Outdated
delete(items, secretRefKey)
typeVal, exists := items[typeKey]
if !exists {
// conversion from v1beta1 is required:
Copy link
Member

Choose a reason for hiding this comment

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

Should the backward compatibility to v1beta1 be done here? Shouldn't this be handled by a webhook or some other mechanism addressing backward compatibility?

Copy link
Member

Choose a reason for hiding this comment

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

What was decided about this @shlomitk1 @roytman ? I see you took the code out, but I don't see the handling of the backward compatibility anyplace else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It still happens during the object construction. Webhooks can come on top of that, but a default conversion is required if no webhooks are running.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that no one uses the vibeta1 version yet, I suggest adding the following properties into its definition:

versions.served: false    # this prevents creation of new instances of v1beta1, but do not return any errors if there are instances of the version. (just in case) 
versions.storage: false  # this is defined in the PR
versions.deprecated: true
versions.deprecationWarning: "a text describe that the version is deprecated, unsupported, will be removed soon, and suggest to use v1beta2

And do not add any conversions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Served is set to true because otherwise deprecated warnings are not displayed.

@shlomitk1
Copy link
Contributor Author
shlomitk1 commented Jan 8, 2023 •
edited
Loading

As @roytman pointed out, this PR has backward compatibility implications. 1) It should be marked as such 2) The storage CRDs in the quick start examples need to be updated, as does the kfp sample 3) Does this affect any of the OPA policies in any of our examples?

It is marked with ⚠️
Backward compatibility is preserved - the older version is supported, and the automatic conversion takes place.
OPA policies are not affected.
Modified samples in documentation. @simanadler Where is kfp sample located? If not in this repo, a new issue should be created there.

@roytman
8000 Copy link
Collaborator
roytman commented Jan 8, 2023

UnmarshalJSON takes care of the conversion to the expected structure.

I'm not sure that it is properly done, can you elaborate ?

@shlomitk1
Copy link
Contributor Author

UnmarshalJSON takes care of the conversion to the expected structure.

I'm not sure that it is properly done, can you elaborate ?

After the automatic conversion takes place (no webhooks exist), I get the following spec structure converted from beta1 to beta2:

spec:
  id: theshire-object-store
  secretRef: credentials-theshire
  region: theshire
  endpoint: http://s3.eu.cloud-object-storage.appdomain.cloud

The code converts this structure to beta2 scheme:

spec:
  id: theshire-object-store
  type: s3
  secretRef: credentials-theshire
  geography: theshire
  s3:
    region: theshire
    endpoint: http://s3.eu.cloud-object-storage.appdomain.cloud

Not regarding the version management, UnmarshalJSON is necessary here because the default unmarshal does not understand unstructured data (there are parts in the spec that are not in the scheme, allowing unknown fields). I added the conversion to the right version during the json decoding to the structure.

@shlomitk1
fix definition
5b7b93b 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
modified samples
309ce9c 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
apis reorder
af7f9a6 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
Merge branch 'master' into sa-crd
86aaa8e
@simanadler
Copy link
Member

As @roytman pointed out, this PR has backward compatibility implications. 1) It should be marked as such 2) The storage CRDs in the quick start examples need to be updated, as does the kfp sample 3) Does this affect any of the OPA policies in any of our examples?

It is marked with ⚠️ Backward compatibility is preserved - the older version is supported, and the automatic conversion takes place. OPA policies are not affected. Modified samples in documentation. @simanadler Where is kfp sample located? If not in this repo, a new issue should be created there.

https://github.com/fybrik/kfp-components/tree/main/samples/house_price_estimates

@shlomitk1
Copy link
Contributor Author

https://github.com/fybrik/kfp-components/tree/main/samples/house_price_estimates

Once this PR is merged, I'll open a PR in kfp-components repo to replace the CR.

roytman
roytman reviewed Jan 11, 2023
View reviewed changes
pkg/storage/apis/app/v1beta2/fybrikstorageaccount_types.go Outdated
delete(items, secretRefKey)
typeVal, exists := items[typeKey]
if !exists {
// conversion from v1beta1 is required:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that no one uses the vibeta1 version yet, I suggest adding the following properties into its definition:

versions.served: false    # this prevents creation of new instances of v1beta1, but do not return any errors if there are instances of the version. (just in case) 
versions.storage: false  # this is defined in the PR
versions.deprecated: true
versions.deprecationWarning: "a text describe that the version is deprecated, unsupported, will be removed soon, and suggest to use v1beta2

And do not add any conversions.

@shlomitk1
PR comments
3d717e3 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
@shlomitk1
Merge remote-tracking branch 'upstream/master' into sa-crd
325662d
@shlomitk1
fix lint issues
b89e2a8 
Signed-off-by: Shlomit Koyfman <shlomitk@il.ibm.com>
roytman
roytman reviewed Jan 11, 2023
View reviewed changes
@shlomitk1 shlomitk1 enabled auto-merge (squash) January 11, 2023 18:18
@shlomitk1 shlomitk1 dismissed simanadler’s stale review January 12, 2023 06:59

The review comments are addressed. We decided to display a warning about the version being deprecated, not support v1beta1 and move the conversion code out.

@shlomitk1 shlomitk1 merged commit bb8eff4 into fybrik:master Jan 12, 2023
@shlomitk1 shlomitk1 deleted the sa-crd branch January 12, 2023 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roytman roytman roytman approved these changes

@simanadler simanadler Awaiting requested review from simanadler

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt FybrikStorageAccount to other connection types
3 participants
@shlomitk1 @roytman @simanadler
0