Skip to content

Conversation

@soenkeliebau
Copy link
Member

@soenkeliebau soenkeliebau commented Feb 8, 2023

Description

This PR is a spike to look into how we can push common behavior into the framework. This is not intended for merging as is, just as basis for further discussions.

This branch depends on a branch of the operator framework.

Specifically in this PR I implemented the following functionality:

I have also introduced a struct that can be shared across all operators from the framework: https://github.com/stackabletech/operator-rs/blob/feat/pause-reconciliation/src/config/common_config.rs#L11

In the operators this can then be flattened into the CRD: https://github.com/stackabletech/hdfs-operator/blob/feat/pause-stop/rust/crd/src/lib.rs#L68

This should be backwards compatible with the existing stopped and serviceType flags in a few of our operators, as the resulting schema looks the same, but will be deserialized to the shared struct (full disclosure: untested as of yet).

I have implemented two possible ways of calling the functionlity from the operator, one with a proc macro and one with regular fn's. The fn way is commented out at the moment, but you can test this very simply by removing the macro call and uncommenting the block below it in hdfs-controller.rs

The branch also contains changes to use Tilt and crate2nix, you can ignore these when looking at the code changes themselves, those were just convenience functions for development.

Review Checklist

  • Code contains useful comments
  • CRD change approved (or not applicable)
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)
  • Helm chart can be installed and deployed operator works (or not applicable)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@soenkeliebau soenkeliebau marked this pull request as draft February 9, 2023 08:38
@fhennig
Copy link
Contributor

fhennig commented Feb 9, 2023

In previous discussions the "pause_reconcile" was done as an annotation, I quite liked that because it's meta (related to the operator workings, not the product) and found it sensible to not include it in the ProductCluster spec. Could we do that?

Other than that, looks nice I think!

@soenkeliebau
Copy link
Member Author

My initial implementation used an annotation as well, yes.
I discussed this with Nat and she convinced me that we should rather put it into the CRD proper.

Basically the argument was that annotations appear to give us more freedom to change things without breaking compatibility - however that is not trully the case, stuff still breaks, but without you noticing, because the apiserver still takes objects of your hands and applies them, but you need to know that something changed in how annotations are parsed by us.
Whereas by putting it in the CRD we force ourselves to properly handle these changes, give us the ability to provide conversions etc.

Also, in the crd ides will support autocomplete, syntax checking etc.

@sbernauer
Copy link
Member

@soenkeliebau I guess this can be closed?

@soenkeliebau
Copy link
Member Author

Aye

@razvan razvan deleted the feat/pause-stop branch November 6, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants