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.

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

podman run - avoid calls to JSONDeepCopy #11781

Merged
merged 4 commits into from
Sep 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 15 additions & 4 deletions libpod/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -774,9 +774,9 @@ func (c *Container) ExecSessions() ([]string, error) {
return ids, nil
}

// ExecSession retrieves detailed information on a single active exec session in
// a container
func (c *Container) ExecSession(id string) (*ExecSession, error) {
// execSessionNoCopy returns the associated exec session to id.
// Note that the session is not a deep copy.
func (c *Container) execSessionNoCopy(id string) (*ExecSession, error) {
if !c.batched {
c.lock.Lock()
defer c.lock.Unlock()
Expand All @@ -791,6 +791,17 @@ func (c *Container) ExecSession(id string) (*ExecSession, error) {
return nil, errors.Wrapf(define.ErrNoSuchExecSession, "no exec session with ID %s found in container %s", id, c.ID())
}

return session, nil
}

// ExecSession retrieves detailed information on a single active exec session in
// a container
func (c *Container) ExecSession(id string) (*ExecSession, error) {
session, err := c.execSessionNoCopy(id)
if err != nil {
return nil, err
}

returnSession := new(ExecSession)
if err := JSONDeepCopy(session, returnSession); err != nil {
return nil, errors.Wrapf(err, "error copying contents of container %s exec session %s", c.ID(), session.ID())
Expand Down Expand Up @@ -1095,7 +1106,7 @@ func (c *Container) AutoRemove() bool {
if spec.Annotations == nil {
return false
}
return c.Spec().Annotations[define.InspectAnnotationAutoremove] == define.InspectResponseTrue
return spec.Annotations[define.InspectAnnotationAutoremove] == define.InspectResponseTrue
}

// Timezone returns the timezone configured inside the container.
Expand Down
2 changes: 1 addition & 1 deletion libpod/container_exec.go
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
return -1, err
}

session, err := c.ExecSession(sessionID)
session, err := c.execSessionNoCopy(sessionID)
if err != nil {
if errors.Cause(err) == define.ErrNoSuchExecSession {
// TODO: If a proper Context is ever plumbed in here, we
Expand Down
4 changes: 2 additions & 2 deletions libpod/container_internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -2004,7 +2004,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (map[s
}
return nil, err
}
ociHooks, err := manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
ociHooks, err := manager.Hooks(config, c.config.Spec.Annotations, len(c.config.UserVolumes) > 0)
if err != nil {
return nil, err
}
Expand All @@ -2021,7 +2021,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (map[s
return nil, err
}

allHooks, err = manager.Hooks(config, c.Spec().Annotations, len(c.config.UserVolumes) > 0)
allHooks, err = manager.Hooks(config, c.config.Spec.Annotations, len(c.config.UserVolumes) > 0)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion libpod/kube.go
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,7 @@ func containerToV1Container(ctx context.Context, c *Container) (v1.Container, []
// NOTE: a privileged container mounts all of /dev/*.
if !c.Privileged() && len(c.config.Spec.Linux.Devices) > 0 {
// TODO Enable when we can support devices and their names
kubeContainer.VolumeDevices = generateKubeVolumeDeviceFromLinuxDevice(c.Spec().Linux.Devices)
kubeContainer.VolumeDevices = generateKubeVolumeDeviceFromLinuxDevice(c.config.Spec.Linux.Devices)
return kubeContainer, kubeVolumes, nil, errors.Wrapf(define.ErrNotImplemented, "linux devices")
}

Expand Down
19 changes: 16 additions & 3 deletions libpod/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -706,19 +706,32 @@ func (r *Runtime) TmpDir() (string, error) {
return r.config.Engine.TmpDir, nil
}

// GetConfig returns a copy of the configuration used by the runtime
func (r *Runtime) GetConfig() (*config.Config, error) {
// GetConfig returns the configuration used by the runtime.
// Note that the returned value is not a copy and must hence
// only be used in a reading fashion.
func (r *Runtime) GetConfigNoCopy() (*config.Config, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little uncomfortable about this one, since it does give a backdoor into the Libpod config. I am OK with merging but would prefer more warnings telling people to avoid whenever possible, and how this is fastpath only, etc.

Copy link
Member

Choose a reason for hiding this comment

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

(If we could get lint to throw errors whenever someone adds a use, requiring a manual whitelisting, that would be neat, but that sounds beyond the scope of this PR)

r.lock.RLock()
defer r.lock.RUnlock()

if !r.valid {
return nil, define.ErrRuntimeStopped
}
return r.config, nil
}

// GetConfig returns a copy of the configuration used by the runtime.
// Please use GetConfigNoCopy() in case you only want to read from
// but not write to the returned config.
func (r *Runtime) GetConfig() (*config.Config, error) {
rtConfig, err := r.GetConfigNoCopy()
if err != nil {
return nil, err
}

config := new(config.Config)

// Copy so the caller won't be able to modify the actual config
if err := JSONDeepCopy(r.config, config); err != nil {
if err := JSONDeepCopy(rtConfig, config); err != nil {
return nil, errors.Wrapf(err, "error copying config")
}

Expand Down
5 changes: 1 addition & 4 deletions libpod/runtime_ctr.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,7 @@ func (r *Runtime) initContainerVariables(rSpec *spec.Spec, config *ContainerConf
ctr.config.LogPath = ""
}

ctr.config.Spec = new(spec.Spec)
if err := JSONDeepCopy(rSpec, ctr.config.Spec); err != nil {
return nil, errors.Wrapf(err, "error copying runtime spec while creating container")
}
ctr.config.Spec = rSpec
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
ctr.config.CreatedTime = time.Now()

ctr.state.BindMounts = make(map[string]string)
Expand Down
2 changes: 1 addition & 1 deletion pkg/domain/infra/abi/secrets.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func (ic *ContainerEngine) SecretCreate(ctx context.Context, name string, reader

// set defaults from config for the case they are not set by an upper layer
// (-> i.e. tests that talk directly to the api)
cfg, err := ic.Libpod.GetConfig()
cfg, err := ic.Libpod.GetConfigNoCopy()
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/specgen/generate/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
}
}

rtc, err := r.GetConfig()
rtc, err := r.GetConfigNoCopy()
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/specgen/generate/container_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import (
// Returns the created, container and any warnings resulting from creating the
// container, or an error.
func MakeContainer(ctx context.Context, rt *libpod.Runtime, s *specgen.SpecGenerator) (*spec.Spec, *specgen.SpecGenerator, []libpod.CtrCreateOption, error) {
rtc, err := rt.GetConfig()
rtc, err := rt.GetConfigNoCopy()
if err != nil {
return nil, nil, nil, err
}
Expand Down