Fix: multi config sync#9958
Fix: multi config sync#9958LudwigNordstrom wants to merge 3 commits intoGoogleContainerTools:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @LudwigNordstrom, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where file synchronization was inefficiently performed multiple times in multi-config projects. It introduces deployment-aware filtering to ensure each syncer only attempts to sync files for images it is responsible for, improving performance and reducing unnecessary operations. The changes include a new interface, updates to the pod syncer and deployers, and a new integration test to validate the fix. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of redundant file sync operations in multi-config Skaffold projects. The introduction of the DeploymentAwareSyncer interface and the corresponding updates to PodSyncer and deployers (kubectl, helm, kpt) ensure that each syncer only handles artifacts deployed by its associated deployer. This is a well-designed solution that improves efficiency and correctness for multi-config setups. The added integration and unit tests provide good coverage for the new functionality.
| // DeploymentAwareSyncer extends Syncer with the ability to track deployed artifacts. | ||
| // By tracking which artifacts each deployer deployed | ||
| // the syncer can skip sync operations for images it didn't deploy. | ||
| type DeploymentAwareSyncer interface { | ||
| Syncer | ||
| RegisterDeployedArtifacts([]graph.Artifact) | ||
| DeployedArtifacts() []graph.Artifact | ||
| } |
There was a problem hiding this comment.
The DeploymentAwareSyncer interface is a good abstraction for the new filtering logic. It clearly separates the concerns of tracking deployed artifacts from the general sync operations.
| func NewPodSyncer(cli *pkgkubectl.CLI, namespaces *[]string, formatter logger.Formatter, deployedArtifacts []graph.Artifact) *PodSyncer { | ||
| return &PodSyncer{ | ||
| kubectl: cli, | ||
| namespaces: namespaces, | ||
| formatter: formatter, | ||
| kubectl: cli, | ||
| namespaces: namespaces, | ||
| formatter: formatter, | ||
| deployedArtifacts: deployedArtifacts, | ||
| } |
There was a problem hiding this comment.
The NewPodSyncer constructor has been correctly updated to accept the deployedArtifacts parameter, ensuring proper initialization of the new filtering mechanism.
| // In multi-config projects, SyncerMux calls Sync on all syncers. To avoid | ||
| // syncing the same file N times, we check if this syncer's deployer actually | ||
| // deployed the image. If not, we skip and let the appropriate syncer handle it. | ||
| if !slices.ContainsFunc(s.DeployedArtifacts(), func(artifact graph.Artifact) bool { | ||
| return artifact.Tag == item.Image | ||
| }) { | ||
| log.Entry(ctx).Infof("Skipping sync for image %q: not in deployed artifacts", item.Image) | ||
| return nil |
There was a problem hiding this comment.
This block correctly implements the deployment-aware filtering. The slices.ContainsFunc check ensures that sync operations are only performed for images that the current syncer's deployer is responsible for, preventing redundant work in multi-config scenarios. The Infof log message is also helpful for debugging.
| // Register with syncer for multi-config sync filtering | ||
| if st, ok := k.syncer.(sync.DeploymentAwareSyncer); ok { | ||
| st.RegisterDeployedArtifacts(deployedImages) | ||
| } |
There was a problem hiding this comment.
The deployer correctly registers its deployed artifacts with the syncer, enabling the new filtering logic. This ensures that each syncer has the necessary context to determine if it should process a sync event.
| // Register with syncer for multi-config sync filtering | ||
| if st, ok := h.syncer.(sync.DeploymentAwareSyncer); ok { | ||
| st.RegisterDeployedArtifacts(deployedImages) | ||
| } |
There was a problem hiding this comment.
The deployer correctly registers its deployed artifacts with the syncer, enabling the new filtering logic. This ensures that each syncer has the necessary context to determine if it should process a sync event.
| // Register with syncer for multi-config sync filtering | ||
| if st, ok := k.syncer.(sync.DeploymentAwareSyncer); ok { | ||
| st.RegisterDeployedArtifacts(deployedImages) | ||
| } |
There was a problem hiding this comment.
The deployer correctly registers its deployed artifacts with the syncer, enabling the new filtering logic. This ensures that each syncer has the necessary context to determine if it should process a sync event.
| if strings.Count(stdErrStr, "Skipping sync for image") != 1 { | ||
| t.Error("Expected to see one 'Skipping sync for image' log message") | ||
| } | ||
|
|
||
| if strings.Count(stdErrStr, "Copying files:") != 1 { | ||
| t.Error("Expected to see one 'Copying files' log message") | ||
| } |
There was a problem hiding this comment.
The assertions for Skipping sync for image and Copying files log messages are crucial for verifying the new filtering behavior in a multi-config setup. This provides strong confidence that the fix is working as intended.
1 participant
Fixes: #7447
Related: N/A
Merge before/after: N/A
Description
When running skaffold dev with file sync enabled in a project with multiple configs, every file sync operation was being attempted N times (once per config). This happened because SyncerMux calls Sync on all registered syncers, and each syncer would attempt to find the corresponding image and sync the file, regardless of whether its deployer was actually responsible for deploying that image.
Solution
Introduced deployment-aware filtering for file syncing: