Skip to content

fix: add applySas #432

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+53 −1
Open

fix: add applySas #432

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
498ff4f
fix: add applySas
LY-today Jun 16, 2025
76f973e
fix: update applySas
LY-today Jun 16, 2025
4ae292a
fix: add sa to DefaultGVRs
LY-today Jun 17, 2025
8bab17c
fix: DefaultGVRs add serviceaccounts
LY-today Jun 23, 2025
bfe50d9
fix: add listSas
LY-today Jun 23, 2025
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
29 changes: 29 additions & 0 deletions simulator/snapshot/snapshot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ type ResourcesForSnap struct {

// ResourcesForLoad indicates all resources and scheduler configuration to be loaded.
type ResourcesForLoad struct {
Sas []v1.ServiceAccountApplyConfiguration `json:"sas"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Now SnapshotService is only used by SnapshotHandler and it only handles resources of the fake cluster. So we don't need to change SnapshotService.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to delete this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will be an exception after deletion

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saza-ku Any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean we don't need the entire fix in SnapshotService, not only the line.

Copy link
Contributor

Choose a reason for hiding this comment

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

instead of the DefaultGVRs of simulator/oneshotimporter/importer.go

No, I mean we need to fix both oneshotimporter and syncer. And this has been achived by 8bab17c. So that's okay!

And I mistakenly believed that SnapshotService didn't need to handle ServiceAccount. But if you export resources that is imported by real clusters, it can contain ServiceAccount that is other than default. Sorry for my misunderstanding.

Copy link
Contributor

@saza-ku saza-ku Jun 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But we need additional fix. It needs to Snap ServiceAccount not only Load.

Copy link
Contributor Author

@LY-today LY-today Jun 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@saza-ku How can I fix it? Currently, all the changes in this PR can run normally on my local machine and meet my needs. Other fixes may require your guidance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Almost the same as Load. You would add Sas to ResourcesForSnap, and implement listSas.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost the same as Load. You would add Sas to ResourcesForSnap, and implement listSas.

done

Pods []v1.PodApplyConfiguration `json:"pods"`
Nodes []v1.NodeApplyConfiguration `json:"nodes"`
Pvs []v1.PersistentVolumeApplyConfiguration `json:"pvs"`
Expand Down Expand Up @@ -173,6 +174,9 @@ func (s *Service) apply(ctx context.Context, resources *ResourcesForLoad, opts o
if err := s.applyNodes(ctx, resources, errgrp, opts); err != nil {
return xerrors.Errorf("call applyNodes: %w", err)
}
if err := s.applySas(ctx, resources, errgrp, opts); err != nil {
return xerrors.Errorf("call applySas: %w", err)
}
if err := s.applyPods(ctx, resources, errgrp, opts); err != nil {
return xerrors.Errorf("call applyPods: %w", err)
}
Expand Down Expand Up @@ -490,6 +494,31 @@ func (s *Service) applyNodes(ctx context.Context, r *ResourcesForLoad, eg *util.
return nil
}

func (s *Service) applySas(ctx context.Context, r *ResourcesForLoad, eg *util.SemaphoredErrGroup, opts options) error {
for i := range r.Sas {
sa := r.Sas[i]
if err := eg.Go(func() error {
sa.ObjectMetaApplyConfiguration.UID = nil
sa.ObjectMetaApplyConfiguration.CreationTimestamp = nil
sa.ObjectMetaApplyConfiguration.ResourceVersion = nil

sa.WithAPIVersion("v1").WithKind("ServiceAccount")

_, err := s.client.CoreV1().ServiceAccounts(*sa.Namespace).Apply(ctx, &sa, metav1.ApplyOptions{Force: true, FieldManager: "simulator"})
if err != nil {
if !opts.ignoreErr {
return xerrors.Errorf("apply Sa: %w", err)
}
klog.Errorf("failed to apply Sa: %v", err)
}
return nil
}); err != nil {
return xerrors.Errorf("start error group: %w", err)
}
}
return nil
}

func (s *Service) applyPods(ctx context.Context, r *ResourcesForLoad, eg *util.SemaphoredErrGroup, opts options) error {
for i := range r.Pods {
pod := r.Pods[i]
Expand Down