download/cache full graph of dependencies at beta validate#5809
download/cache full graph of dependencies at beta validate#5809jbw976 merged 5 commits intocrossplane:masterfrom
beta validate#5809Conversation
ezgidemirel
left a comment
ezgidemirel
left a comment
There was a problem hiding this comment.
Thanks @enesonus for the PR, LGTM!
cmd/crank/beta/validate/manager.go
Outdated
| m.deps[image] = true | ||
|
|
||
| if _, ok := m.confs[image]; !ok && dep.Configuration != nil { | ||
| m.confs[image] = true |
There was a problem hiding this comment.
it looks like this line is adding a new element to the m.confs map, while we are currently iterating through it (starting on line 173). This can be problematic with undefined behavior: https://stackoverflow.com/a/68639401
This is demonstrated in https://go.dev/play/p/V23Q7zkItEF - try running that multiple times and you see that you get inconsistent results.
Is the intent here that this iteration will also include the item we are adding here? If so, I don't think we can rely on that. What do you think of this scenario? 🤔
There was a problem hiding this comment.
With more testing now, I think this is an actual problem if there is a 3rd level of dependencies in the tree, e.g. like xpkg.upbound.io/upbound/platform-ref-azure:v0.11.0 has. If there are only 2 levels of dependencies, then they can be added here and it doesn't matter if the iteration misses them because they have no further dependencies to find. However, if there is a 3rd level, and we miss a package at the 2nd level because we've added it to the map during our iteration of it, we will miss those 3rd+ level dependencies.
You can see this by running the commands in https://gist.github.com/jbw976/51d81caf0ece87e2bb6c64418ece8a8d#file-inconsistent-behavior for platform-ref-azure multiple times - you get an inconsistent number of dependencies found/downloaded each time. For example:
❯ export CCACHE="/Users/Jared/.crossplane/cache"; rm -fr "${CCACHE}"; go run cmd/crank/main.go beta validate ~/Desktop/crossplane/test/5809/config-plat-ref-azure.yaml ~/Desktop/crossplane/test/5809/resources --cache-dir="${CCACHE}" | tee cmd-out.txt; cat cmd-out.txt | wc -l
<packages ommitted>
15
> <run again>
13
> <run again>
11
There was a problem hiding this comment.
Hmm yes it looks like we have a bug here. I will be changing it to a new recursive approach that adds configurations and their dependencies.
There was a problem hiding this comment.
Hopefully this problematic situation is fixed with the recursive approach introduced by 70cc678
Awesome, that is definitely helpful @enesonus! If I could humbly provide a bit more opinion on how you've included these testing scenarios in the PR... 🙇♂️ 😇 The commands you have look similar to: I'm not sure everyone 100% agrees with me 😅, but I am very appreciative when I come to review a PR and I don't have to think or put much effort into recreating the testing scenarios the author is describing. I'm a bit slow 🐢, so it's ideal at least for me when the testing scenarios can be completely copy/pasted without any further work or thinking required at least to run them. Certainly there is thinking needed to evaluate and verify them, but getting the effort to run them down to 0 is really great. All that to say - do you have examples of what is in your Does that make sense? Other maintainers may not be as lazy, yet still thorough, as me 😂 EDIT: I found a bit more time today, so I got the testing scenario set up myself too - it wasn't too hard 😉. Basically, My setup/commands can be found in https://gist.github.com/jbw976/51d81caf0ece87e2bb6c64418ece8a8d |
jbw976
left a comment
jbw976
left a comment
There was a problem hiding this comment.
I got a chance to do further testing and it looks like there is a bug here for deeper dependency trees - do you want to take a deeper look to check my findings @enesonus, and then if needed update this PR with a fix? thank you! 🙇♂️
cmd/crank/beta/validate/manager.go
Outdated
| m.deps[image] = true | ||
|
|
||
| if _, ok := m.confs[image]; !ok && dep.Configuration != nil { | ||
| m.confs[image] = true |
There was a problem hiding this comment.
With more testing now, I think this is an actual problem if there is a 3rd level of dependencies in the tree, e.g. like xpkg.upbound.io/upbound/platform-ref-azure:v0.11.0 has. If there are only 2 levels of dependencies, then they can be added here and it doesn't matter if the iteration misses them because they have no further dependencies to find. However, if there is a 3rd level, and we miss a package at the 2nd level because we've added it to the map during our iteration of it, we will miss those 3rd+ level dependencies.
You can see this by running the commands in https://gist.github.com/jbw976/51d81caf0ece87e2bb6c64418ece8a8d#file-inconsistent-behavior for platform-ref-azure multiple times - you get an inconsistent number of dependencies found/downloaded each time. For example:
❯ export CCACHE="/Users/Jared/.crossplane/cache"; rm -fr "${CCACHE}"; go run cmd/crank/main.go beta validate ~/Desktop/crossplane/test/5809/config-plat-ref-azure.yaml ~/Desktop/crossplane/test/5809/resources --cache-dir="${CCACHE}" | tee cmd-out.txt; cat cmd-out.txt | wc -l
<packages ommitted>
15
> <run again>
13
> <run again>
11
These are very valuable feedbacks on my side @jbw976! I wasn't very aware of how powerful was the github gists. I tried to make PR description as brief as possible so I did not add details. I will try to create a gist for this PR on how I was testing it. |
enesonus
force-pushed
the
download-all-config-deps
branch
from
82b25e0 to
70cc678
Compare
|
I changed the dependency addition function I created a script that runs the |
|
Very cool @enesonus, I was able to use your gist and testing script to get reliable download results as well. Looks like this approach is working well! Quick question about the test script: https://gist.github.com/enesonus/481dd30a911f6e27c1d03c52d0fad1aa#file-test_add_dep-sh-L24. What does that kubectl context check for? I don't see |
TBH I dont really know what it actually affects 😅 At my initial run of script I got: I tried some different things and somehow it got fixed. I didn't understand what fixed it so did not want to remove it and possibly break it again 😅 I tried running it on a different computer, it was running so I didnt spend much time thinking on the script's code :) |
enesonus
force-pushed
the
download-all-config-deps
branch
from
70cc678 to
089a5df
Compare
| confs: 2, // 1 Base Configuration, 1 Child Configuration | ||
| deps: 4, // 2 Configurations, 1 provider, 1 function |
There was a problem hiding this comment.
listing out the names of each would be a bit more helpful to understand here, e.g.:
// 1 Base configuration (config-dep-1), 1 child configuration (config-dep-2)
// 2 configurations (config-dep-1, config-dep-2), 1 provider (provider-dep-1), 1 function (function-dep-1)
| fs := afero.NewMemMapFs() | ||
| w := &bytes.Buffer{} | ||
|
|
||
| m := NewManager(".crossplane/cache", fs, w) |
There was a problem hiding this comment.
does the global cache change from #5839 affect this cache usage in unit tests? i.e. if this will no longer be a relative path, where does it go? 🤔
There was a problem hiding this comment.
Actually this cache is not even used since we get the yaml files from a MockFetcher. Normally this would create .crossplane/cache directory at the current directory and use it as a cache for next calls but we dont use it. I will change this to "" to prevent misunderstandings.
Signed-off-by: Mehmet Enes <menes.onus@gmail.com>
Signed-off-by: Mehmet Enes <menes.onus@gmail.com>
Signed-off-by: Mehmet Enes <menes.onus@gmail.com>
Co-authored-by: Jared Watts <jbw976@gmail.com> Signed-off-by: Mehmet Enes <94247411+enesonus@users.noreply.github.com>
enesonus
force-pushed
the
download-all-config-deps
branch
from
3b3be23 to
a6632ac
Compare
|
Ah darn @enesonus, was just about to merge this but it looks like the last commit is failing the DCO check: Could you amend that commit with the sign-off (https://github.com/crossplane/crossplane/tree/master/contributing#certificate-of-origin), then we can merge? 🙏 |
Signed-off-by: Mehmet Enes <menes.onus@gmail.com>
enesonus
force-pushed
the
download-all-config-deps
branch
from
a6632ac to
93dff5d
Compare
|
Ahh sorry, I changed my laptop and I forgot to configure auto sign 😅 It should be good to go now |
3 participants
Description of your changes
This PR enables 'beta validate' command to calculate all dependencies of the provided configurations and therefore the full graph of dependencies is downloaded/cached. Previously we were only downloading/caching the level-1 dependencies of a configuration which are the dependencies of the provided configuration..
Fixes #5803
Here are some examples with this functionality:
configuration-aws-database:v0.10.0Dependency graph is:
provider-aws-rdsconfiguration-aws-network:v0.12.0provider-aws-ec2:v1.2.0function-patch-and-transform:v0.4.0Current behavior:
New behavior:
platform-ref-azure:v0.11.0:Dependency graph is:
provider-azure-containerservice:v0.42.1configuration-azure-network:v0.8.0provider-azure-network:v0.42.1function-patch-and-transform:v0.4.0configuration-azure-aks:v0.7.0provider-azure-containerservice:v0.42.1configuration-azure-network:v0.8.0provider-azure-network:v0.42.1function-patch-and-transform:v0.4.0provider-helm:v0.17.0provider-kubernetes:v0.12.1configuration-azure-database:v0.11.1provider-azure-dbformariadb:v0.42.1provider-azure-dbforpostgresql:v0.42.1configuration-azure-network:v0.8.0provider-azure-network:v0.42.1function-patch-and-transform:v0.4.0function-patch-and-transform:v0.4.0configuration-app:v0.5.0provider-helm:v0.17.0function-patch-and-transform:v0.4.0configuration-observability-oss:v0.5.0provider-helm:v0.17.0provider-kubernetes:v0.12.1provider-grafana:v0.8.0function-patch-and-transform:v0.4.0configuration-gitops-flux:v0.6.0provider-helm:v0.17.0function-patch-and-transform:v0.4.0This configuration has 14 distinct dependencies. We need to download it's own yaml plus 14 other yamlsCurrent behavior:
New behavior:
I have:
earthly +reviewableto ensure this PR is ready for review.Added or updated e2e tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.