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

WIP: Test XP 1.14 Usage kind #113

Closed
wants to merge 6 commits into from

Conversation

stevendborrelli
Copy link
Member

@stevendborrelli stevendborrelli commented Aug 30, 2023

Description of your changes

This PR utilizes the Usage tracking being added to Crossplane 1.14 in crossplane/crossplane#4444. Do not merge until the release of 1.14.

To install a compatible version of Crossplane, run:

helm repo add crossplane-stable https://charts.crossplane.io/stable && helm repo update crossplane-stable

helm upgrade --install crossplane --namespace crossplane-system crossplane-stable/crossplane --version v1.13.2 --create-namespace --set image.repository=turkenh/crossplane
 --set image.tag=v1.14.0-rc.0.198.g9049ab9c --set "args={--debug,--enable-usages, --enable-environment-configs}"

Fixes #

I have:

  • Read and followed Upbound's contribution process.
  • Run make reviewable to ensure this PR is ready for review.
  • Added backport release-x.y labels to auto-backport this PR, as appropriate.

How has this code been tested

Work in progress.

Signed-off-by: Steven Borrelli <steve@borrelli.org>
turkenh and others added 4 commits August 31, 2023 14:10
Signed-off-by: Hasan Turken <turkenh@gmail.com>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli stevendborrelli self-assigned this Aug 31, 2023
kind: XEKS
by:
apiVersion: helm.crossplane.io/v1beta1
kind: Release
Copy link
Member

Choose a reason for hiding this comment

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

Is it a direct dependency on Release MR, not XServices XR to avoid Foreground deletion style, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I may test it against the Composite again with foreground deletion enabled.

Copy link
Member Author

@stevendborrelli stevendborrelli Aug 31, 2023

Choose a reason for hiding this comment

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

I don't think it will work across Composites (without a claim). I tested using defaultCompositeDeletePolicy and it seemingly does not work on XRs that are created without a Claim (as in XServices), so XService gets deleted immediately and takes the Usage and XEKS with it, leaving the Release.helm unable to delete.

Copy link
Member Author

@stevendborrelli stevendborrelli Sep 1, 2023

Choose a reason for hiding this comment

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

Setting compositeDeletePolicy: Foreground in the Cluster Claim seems to work with a usage between composites. I'll keep testing.

Signed-off-by: Steven Borrelli <steve@borrelli.org>
@stevendborrelli
Copy link
Member Author

Updated to use Foreground deletion on the claim so that the usage is between composites. I left the older example of the MR usage in the code commented out for now.

kubectl get usages
NAME                         DETAILS                                                                     READY   AGE
borrelli-usage-2brd6-v8zmq   XServices/borrelli-usage-2brd6-k86mj uses XEKS/borrelli-usage-2brd6-gjvx7   True    4m36s

@@ -3,6 +3,7 @@ kind: CompositeResourceDefinition
metadata:
name: xclusters.aws.platformref.upbound.io
spec:
defaultCompositeDeletePolicy: Foreground
Copy link
Member

Choose a reason for hiding this comment

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

Would this be sufficient without passing a special flag to kubectl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but only if we have a claim applied. It will not work if we create the XR without a claim.

Copy link
Member Author

Choose a reason for hiding this comment

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

I confirmed via testing an XR that even if the XRD has defaultCompositeDeletePolicy: Foreground set, it is not set unless a Claim is provisioned.

@turkenh
Copy link
Member

turkenh commented Sep 7, 2023

Updated to use Foreground deletion on the claim so that the usage is between composites. I left the older example of the MR usage in the code commented out for now.

kubectl get usages
NAME                         DETAILS                                                                     READY   AGE
borrelli-usage-2brd6-v8zmq   XServices/borrelli-usage-2brd6-k86mj uses XEKS/borrelli-usage-2brd6-gjvx7   True    4m36s

I believe this wouldn't work if someone wants to interact with composites directly (unless they pass a special flag to kubectl delete), right?

I think we should rather go with the solution that would work for both background/foreground deletion policies, not to promote this in our reference platforms and cause confusions later.

@stevendborrelli
Copy link
Member Author

Updated to use Foreground deletion on the claim so that the usage is between composites. I left the older example of the MR usage in the code commented out for now.

kubectl get usages
NAME                         DETAILS                                                                     READY   AGE
borrelli-usage-2brd6-v8zmq   XServices/borrelli-usage-2brd6-k86mj uses XEKS/borrelli-usage-2brd6-gjvx7   True    4m36s

I believe this wouldn't work if someone wants to interact with composites directly (unless they pass a special flag to kubectl delete), right?

I think we should rather go with the solution that would work for both background/foreground deletion policies, not to promote this in our reference platforms and cause confusions later.

I guess my issue with this is one of encapsulation, for example in this code we have an MR inside a composition creating a Usage to another XR. I'm uncomfortable with managed resources inside an Composition knowing specifics of the environment outside. The XServices XR might be used by other XRs.

If we have the MR inside XServices refer to something like another MR, (like a Cluster.eks.aws.upbound.io, we still have an issue that we need to know the GVK of the Kubernetes cluster the Helm release is deploying against.

    - name: release-uxp-uses-xeks
      base:
        apiVersion: apiextensions.crossplane.io/v1alpha1
        kind: Usage
        spec:
          of:
            apiVersion: aws.platformref.upbound.io/v1alpha1
            kind: XEKS
          by:
            apiVersion: helm.crossplane.io/v1beta1
            kind: Release

@haarchri haarchri mentioned this pull request Oct 15, 2023
3 tasks
@haarchri
Copy link
Member

@stevendborrelli thanks - i implemented this change + XApps in #116

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.

4 participants