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
feat(crank): add beta trace command #4849
feat(crank): add beta trace command #4849
Conversation
7aa7104
to
7452c0c
Compare
Suggestions about the name of the command are welcome. |
+1 to |
If the output is the same or close to |
noticed the tree view requires a bit more refinement |
7452c0c
to
e357a5a
Compare
ce4b629
to
91096fa
Compare
From the feedbacks here and I've collected irl, sounds like we have some consensus around I added back the Right now this is only able to handle XRC/XR/MR, we can think of adding Packages and XRDs support down the line. |
Could we extend help for the command to also include the usage of the |
e96db07
to
f16c1f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great progress @phisco!! I took it for a test spin this morning and found a couple issues that aren't working for my test scenario I wanted to debug with this new command. I also had a bit of trouble with the resource name format.
What do you think about my experience and any potential improvements we can make to smooth it out for other folks new to the command? 😊
bf52bcf
to
eb1b6d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking pretty good to me @phisco, I really like where this is landing to ship in v1.14 and then we can continue to iterate with more functionality.
I've added a few comments and questions, but I don't think there is anything blocking on my side, so I'm adding an approval and you can finish this PR off as you want 🤩
// collect owner references | ||
var refs []v1.ObjectReference | ||
|
||
xrc := claim.Unstructured{Unstructured: obj} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we making an assumption here that the passed in resource is a claim (xrc)? it looks more like we treat it like a claim and see what we get from it, then we treat it like a XR and see what we get out of it. is that correct? 🤔
we see more of that ambiguity further down when we're looking up the connection secrets and we're not sure if the object is a claim or XR. I don't have specific suggestions on anything to change here, or even that anything needs to change, but it feels a bit odd that at multiple points in this function we're not sure what type we're working with. You can choose to do something with that observation or not 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I adopted the same approach we are using to build the related resource graph from the e2es, but we could probably make it more explicit by checking the resource labels, I'll give it a shot 👍
// Copied over from cli-runtime pkg/resource Builder, | ||
// https://github.com/kubernetes/cli-runtime/blob/9a91d944dd43186c52e0162e12b151b0e460354a/pkg/resource/builder.go#L768 | ||
func (kc *Client) MappingFor(resourceOrKindArg string) (*meta.RESTMapping, error) { | ||
// TODO(phisco): actually use the Builder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the type of TODO that needs a tracking issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's more a wish, there is no real need. It would be awesome to be able to leverage the upstream code, but it shouldn't change the end result, so I wouldn't actively track it.
If needed the resource kind can be also specified further, | ||
'TYPE[.VERSION][.GROUP]', e.g. mykind.example.org. | ||
|
||
Examples: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love all these examples, that will be really helpful for folks to use this command effectively!! 😎
Just tested with the latest state with platform-ref-aws:v0.6.0 and XP v1.13.2. In the beginning, the command failed with: ./_output/bin/darwin_arm64/crank beta trace cluster platform-ref-aws-ht
crossplane: error: cannot get requested resource: couldn't get child resource: couldn't get resource: resource name may not be empty While I was trying to debug it, it resolved by itself, and started printing the tree. Hence, I don't have much clue about what was wrong before, but enhancing the error message above with some more details could help. Now, I am seeing a proper output as below: ❯ ./_output/bin/darwin_arm64/crank beta trace cluster platform-ref-aws-ht
NAME READY SYNCED STATUS
Cluster/platform-ref-aws-ht (default) True True Available
└─ XCluster/platform-ref-aws-ht-ctk5x True True Available
├─ XNetwork/platform-ref-aws-ht-ctk5x-6lpkx True True Available
│ ├─ VPC/platform-ref-aws-ht-ctk5x-fcx5v True True Available
│ ├─ InternetGateway/platform-ref-aws-ht-ctk5x-5lt6p True True Available
│ ├─ Subnet/platform-ref-aws-ht-ctk5x-b98lz True True Available
│ ├─ Subnet/platform-ref-aws-ht-ctk5x-6n5j8 True True Available
│ ├─ Subnet/platform-ref-aws-ht-ctk5x-fp6r9 True True Available
│ ├─ Subnet/platform-ref-aws-ht-ctk5x-fhwqv True True Available
│ ├─ RouteTable/platform-ref-aws-ht-ctk5x-f9x5v True True Available
│ ├─ Route/platform-ref-aws-ht-ctk5x-qr9db True True Available
│ ├─ MainRouteTableAssociation/platform-ref-aws-ht-ctk5x-64mg4 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-ctk5x-cx9ns True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-ctk5x-vnvfl True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-ctk5x-sr6g7 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-ctk5x-b6zmz True True Available
│ ├─ SecurityGroup/platform-ref-aws-ht-ctk5x-q5nx7 True True Available
│ ├─ SecurityGroupRule/platform-ref-aws-ht-ctk5x-lgpnx True True Available
│ └─ SecurityGroupRule/platform-ref-aws-ht-ctk5x-xsqlb True True Available
├─ XEKS/platform-ref-aws-ht-ctk5x-svcm2 True True Available
│ ├─ Role/platform-ref-aws-ht-ctk5x-5ffp2 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-ctk5x-rwdmg True True Available
│ ├─ Cluster/platform-ref-aws-ht-ctk5x-59f5j True True Available
│ ├─ ClusterAuth/platform-ref-aws-ht-ctk5x-pwkr9 True True Available
│ ├─ Role/platform-ref-aws-ht-ctk5x-hxtz2 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-ctk5x-phvxp True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-ctk5x-pr272 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-ctk5x-hglzh True True Available
│ ├─ NodeGroup/platform-ref-aws-ht-ctk5x-mkfrv True True Available
│ ├─ OpenIDConnectProvider/platform-ref-aws-ht-ctk5x-mrt9x True True Available
│ └─ ProviderConfig/platform-ref-aws-ht Unknown Unknown
└─ XServices/platform-ref-aws-ht-ctk5x-lwgg2 True True Available
├─ Release/platform-ref-aws-ht-ctk5x-5hh8r True True Available
└─ Release/platform-ref-aws-ht-ctk5x-bk7d6 True True Available The only question is whether we could handle |
3fe8cc5
to
8bf644b
Compare
@turkenh |
8416065
to
b841d01
Compare
I've added support for EnvironmentConfigs for XRs too, I think it could help with the environment debugging issues we have been discussing, here a really symptomatic example from our E2Es:
WDYT @jbw976? EDIT: discussing with @turkenh we agreed it didn't make much sense to show selected EnvironmentConfigs as it would be confusing if the selected composition was also creating EnvironmentConfigs, as they would all show on the same level. |
Tested again with the latest master of platform-ref-aws and Crossplane from this commit. First of all, great work 💪 Thank you both @phisco and @jbasement! It will really be a game-changer in terms of debugging complex composites! This is what I get just before being ready, see how easy to see why the claim is not ready 🤩 ❯ ./_output/bin/darwin_arm64/crank beta trace cluster platform-ref-aws-ht
NAME READY SYNCED STATUS
Cluster/platform-ref-aws-ht (default) False True Waiting: ...resource claim is waiting for composite resource to become Ready
└─ XCluster/platform-ref-aws-ht-x22s4 False True Creating: Unready resources: compositeClusterEKS
├─ XNetwork/platform-ref-aws-ht-x22s4-k6v5r True True Available
│ ├─ VPC/platform-ref-aws-ht-x22s4-dflgv True True Available
│ ├─ InternetGateway/platform-ref-aws-ht-x22s4-6j95q True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-ndgzp True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-66n5t True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-z5k8k True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-hc7h2 True True Available
│ ├─ RouteTable/platform-ref-aws-ht-x22s4-6q2g6 True True Available
│ ├─ Route/platform-ref-aws-ht-x22s4-66prw True True Available
│ ├─ MainRouteTableAssociation/platform-ref-aws-ht-x22s4-jttw2 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-zwnb9 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-8qq4z True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-fgqtn True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-r7tfz True True Available
│ ├─ SecurityGroup/platform-ref-aws-ht-x22s4-gzs7p True True Available
│ ├─ SecurityGroupRule/platform-ref-aws-ht-x22s4-h6wbx True True Available
│ └─ SecurityGroupRule/platform-ref-aws-ht-x22s4-znrb7 True True Available
├─ XEKS/platform-ref-aws-ht-x22s4-554vh False True Creating: Unready resources: ebsCsiAddon
│ ├─ Role/platform-ref-aws-ht-x22s4-gvs2d True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-5jgs8 True True Available
│ ├─ Cluster/platform-ref-aws-ht-x22s4-p7z6j True True Available
│ ├─ ClusterAuth/platform-ref-aws-ht-x22s4-q67wj True True Available
│ ├─ Role/platform-ref-aws-ht-x22s4-fhld5 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-c5484 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-4wwzv True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-jsk74 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-tl6fs True True Available
│ ├─ NodeGroup/platform-ref-aws-ht-x22s4-wx9x6 True True Available
│ ├─ Addon/platform-ref-aws-ht-x22s4-prlx8 False True Creating
│ ├─ OpenIDConnectProvider/platform-ref-aws-ht-x22s4-bcccr True True Available
│ ├─ ProviderConfig/platform-ref-aws-ht Unknown Unknown
│ ├─ ProviderConfig/platform-ref-aws-ht Unknown Unknown
│ ├─ Object/platform-ref-aws-ht-irsa-settings True True Available
│ └─ Object/platform-ref-aws-ht-aws-auth True True Available
├─ XServices/platform-ref-aws-ht-x22s4-gpzd2 True True Available
│ └─ Release/platform-ref-aws-ht-x22s4-6rgpx True True Available
└─ Usage/platform-ref-aws-ht-x22s4-s8c9k True Unknown Available And once it is ready, this is what I getting: ❯ ./_output/bin/darwin_arm64/crank beta trace cluster platform-ref-aws-ht
NAME READY SYNCED STATUS
Cluster/platform-ref-aws-ht (default) True True Available
└─ XCluster/platform-ref-aws-ht-x22s4 True True Available
├─ XNetwork/platform-ref-aws-ht-x22s4-k6v5r True True Available
│ ├─ VPC/platform-ref-aws-ht-x22s4-dflgv True True Available
│ ├─ InternetGateway/platform-ref-aws-ht-x22s4-6j95q True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-ndgzp True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-66n5t True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-z5k8k True True Available
│ ├─ Subnet/platform-ref-aws-ht-x22s4-hc7h2 True True Available
│ ├─ RouteTable/platform-ref-aws-ht-x22s4-6q2g6 True True Available
│ ├─ Route/platform-ref-aws-ht-x22s4-66prw True True Available
│ ├─ MainRouteTableAssociation/platform-ref-aws-ht-x22s4-jttw2 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-zwnb9 True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-8qq4z True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-fgqtn True True Available
│ ├─ RouteTableAssociation/platform-ref-aws-ht-x22s4-r7tfz True True Available
│ ├─ SecurityGroup/platform-ref-aws-ht-x22s4-gzs7p True True Available
│ ├─ SecurityGroupRule/platform-ref-aws-ht-x22s4-h6wbx True True Available
│ └─ SecurityGroupRule/platform-ref-aws-ht-x22s4-znrb7 True True Available
├─ XEKS/platform-ref-aws-ht-x22s4-554vh True True Available
│ ├─ Role/platform-ref-aws-ht-x22s4-gvs2d True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-5jgs8 True True Available
│ ├─ Cluster/platform-ref-aws-ht-x22s4-p7z6j True True Available
│ ├─ ClusterAuth/platform-ref-aws-ht-x22s4-q67wj True True Available
│ ├─ Role/platform-ref-aws-ht-x22s4-fhld5 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-c5484 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-4wwzv True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-jsk74 True True Available
│ ├─ RolePolicyAttachment/platform-ref-aws-ht-x22s4-tl6fs True True Available
│ ├─ NodeGroup/platform-ref-aws-ht-x22s4-wx9x6 True True Available
│ ├─ Addon/platform-ref-aws-ht-x22s4-prlx8 True True Available
│ ├─ OpenIDConnectProvider/platform-ref-aws-ht-x22s4-bcccr True True Available
│ ├─ ProviderConfig/platform-ref-aws-ht Unknown Unknown
│ ├─ ProviderConfig/platform-ref-aws-ht Unknown Unknown
│ ├─ Object/platform-ref-aws-ht-irsa-settings True True Available
│ └─ Object/platform-ref-aws-ht-aws-auth True True Available
├─ XServices/platform-ref-aws-ht-x22s4-gpzd2 True True Available
│ └─ Release/platform-ref-aws-ht-x22s4-6rgpx True True Available
└─ Usage/platform-ref-aws-ht-x22s4-s8c9k True Unknown Available The only improvement I can think of is, a better handling for
|
@turkenh, awesome! About the So, should we map empty condition status, |
I think both resources I am interested in here, ProviderConfigs and Usages, don't have those conditions at all. So, seeing |
Then it shouldn't show them as
|
The
But that's OK, it doesn't have a
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, just a few nits!
│ ├─ User/test-resource-child-1-bucket-hash False True SomethingWrongHappened: Error with bucket child 1 | ||
│ ├─ User/test-resource-child-mid-bucket-hash True False CantSync: Sync error with bucket child mid | ||
│ └─ User/test-resource-child-2-bucket-hash False True SomethingWrongHappened: Error with bucket child 2 | ||
│ └─ User/test-resource-child-2-1-bucket-hash - True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should we still have a test for the real Unknown
s :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done: f8998ce
Co-authored-by: jbasement <j.keller@celonis.com> Co-authored-by: Philippe Scorsolini <p.scorsolini@gmail.com> Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
f8998ce
to
9b90e6f
Compare
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
9b90e6f
to
ac99fe0
Compare
Description of your changes
Partially address #4672
Rework based on the work done by @jbasement on #4751, unfortunately the original branch fell so much behind it was really hard to preserve a meaningful history, so as agreed I've squashed everything.
I have:
make reviewable
to ensure this PR is ready for review.Added or updated e2e tests.Addedbackport release-x.y
labels to auto-backport this PR.Need help with this checklist? See the cheat sheet.