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.

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

Improve startup speed #821

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+25 −1
Draft

Improve startup speed #821

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion pkg/config/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/pkg/errors"

"github.com/vmware-tanzu/tanzu-plugin-runtime/config"
"github.com/vmware-tanzu/tanzu-plugin-runtime/config/types"
)

// SyncContextsAndServers populate or sync contexts and servers
Expand All @@ -16,7 +17,11 @@ func SyncContextsAndServers() error {
return errors.Wrap(err, "failed to get client config")
}

config.PopulateContexts(cfg)
hasChanged := config.PopulateContexts(cfg)
needsSync := doServersNeedUpdate(cfg)
if !hasChanged && !needsSync {
return nil
}

// Now write the context to the configuration file. This will also create any missing server for its corresponding context
for _, c := range cfg.KnownContexts {
Copy link
Contributor

Choose a reason for hiding this comment

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

This optimization is quite clean and much appreciated.

I am not sure if we had in the past rely on this to update existing (not create new) entry to correct for any inconsistencies. cc @prkalle
If we want to be really safe, we can consider some undocumented env var to restore back the old behavior for any user who needs it.

Copy link
Contributor Author

@marckhouzam marckhouzam Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at the #616 and what was being done before, I see that we were calling https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/7278b2554915a10ad3abc9e8befe0a3eb196e1a3/config/legacy_clientconfig_factory.go#L21. I based my check on that code to know if there was any servers needing to be updated. So I feel we are covered on that front.

However, there is another aspect to consider. When saving the configuration to file I notice that there is some cleanup of "legacyDiscardedContexts" done here: https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/7278b2554915a10ad3abc9e8befe0a3eb196e1a3/config/config_factory.go#L144-L152

To reproduce the situation I installed cluster:k8s:v0.25.4 and simply ran tanzu cluster version. What I see is that the config.yaml file now has a contexts node with all the servers included there. Before this PR, that contexts node would be removed on the next CLI run. With this PR it is only cleaned up whenever the configuration is changed later on.

I don't think keeping this contexts node is problematic, but let's discuss it

Expand All @@ -36,3 +41,22 @@ func SyncContextsAndServers() error {
}
return nil
}

func doServersNeedUpdate(cfg *types.ClientConfig) bool {
if cfg == nil {
return false
}

for _, c := range cfg.KnownContexts {
if c.ContextType == types.ContextTypeTanzu || cfg.HasServer(c.Name) { //nolint:staticcheck // Deprecated
// context of type "tanzu" don't get synched
// or context already present in servers; skip
continue
}
// Found a context that is not in the servers. We need to update
// the servers section
return true
}

return false
}
Loading