Skip to content

Remove the ToOCI functions from the specs-go package #208

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

Merged
merged 1 commit into from
Jun 10, 2024
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
34 changes: 24 additions & 10 deletions pkg/cdi/container-edits.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error {
if err != nil {
return err
}
dev := d.ToOCI()
dev := dn.toOCI()
if dev.UID == nil && spec.Process != nil {
if uid := spec.Process.User.UID; uid > 0 {
dev.UID = &uid
Expand All @@ -116,29 +116,30 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error {
if len(e.Mounts) > 0 {
for _, m := range e.Mounts {
specgen.RemoveMount(m.ContainerPath)
specgen.AddMount(m.ToOCI())
specgen.AddMount((&Mount{m}).toOCI())
}
sortMounts(&specgen)
}

for _, h := range e.Hooks {
ociHook := (&Hook{h}).toOCI()
switch h.HookName {
case PrestartHook:
specgen.AddPreStartHook(h.ToOCI())
specgen.AddPreStartHook(ociHook)
case PoststartHook:
specgen.AddPostStartHook(h.ToOCI())
specgen.AddPostStartHook(ociHook)
case PoststopHook:
specgen.AddPostStopHook(h.ToOCI())
specgen.AddPostStopHook(ociHook)
// TODO: Maybe runtime-tools/generate should be updated with these...
case CreateRuntimeHook:
ensureOCIHooks(spec)
spec.Hooks.CreateRuntime = append(spec.Hooks.CreateRuntime, h.ToOCI())
spec.Hooks.CreateRuntime = append(spec.Hooks.CreateRuntime, ociHook)
case CreateContainerHook:
ensureOCIHooks(spec)
spec.Hooks.CreateContainer = append(spec.Hooks.CreateContainer, h.ToOCI())
spec.Hooks.CreateContainer = append(spec.Hooks.CreateContainer, ociHook)
case StartContainerHook:
ensureOCIHooks(spec)
spec.Hooks.StartContainer = append(spec.Hooks.StartContainer, h.ToOCI())
spec.Hooks.StartContainer = append(spec.Hooks.StartContainer, ociHook)
default:
return fmt.Errorf("unknown hook name %q", h.HookName)
}
Expand All @@ -148,7 +149,7 @@ func (e *ContainerEdits) Apply(spec *oci.Spec) error {
// The specgen is missing functionality to set all parameters so we
// just piggy-back on it to initialize all structs and the copy over.
specgen.SetLinuxIntelRdtClosID(e.IntelRdt.ClosID)
spec.Linux.IntelRdt = e.IntelRdt.ToOCI()
spec.Linux.IntelRdt = (&IntelRdt{e.IntelRdt}).toOCI()
}

for _, additionalGID := range e.AdditionalGIDs {
Expand Down Expand Up @@ -186,7 +187,7 @@ func (e *ContainerEdits) Validate() error {
}
}
if e.IntelRdt != nil {
if err := ValidateIntelRdt(e.IntelRdt); err != nil {
if err := (&IntelRdt{e.IntelRdt}).Validate(); err != nil {
return err
}
}
Expand Down Expand Up @@ -321,8 +322,21 @@ func (m *Mount) Validate() error {
return nil
}

// IntelRdt is a CDI IntelRdt wrapper.
// This is used for validation and conversion to OCI specifications.
type IntelRdt struct {
*specs.IntelRdt
}

// ValidateIntelRdt validates the IntelRdt configuration.
//
// Deprecated: ValidateIntelRdt is deprecated use IntelRdt.Validate() instead.
func ValidateIntelRdt(i *specs.IntelRdt) error {
return (&IntelRdt{i}).Validate()
}

// Validate validates the IntelRdt configuration.
func (i *IntelRdt) Validate() error {
// ClosID must be a valid Linux filename
if len(i.ClosID) >= 4096 || i.ClosID == "." || i.ClosID == ".." || strings.ContainsAny(i.ClosID, "/\n") {
return errors.New("invalid ClosID")
Expand Down
65 changes: 65 additions & 0 deletions pkg/cdi/oci.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/*
Copyright © 2021 The CDI Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cdi
Copy link
Contributor Author

@elezar elezar May 28, 2024

Choose a reason for hiding this comment

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

@klihub @marquiz since moving these functions here is a breaking change, do we want to add "empty" implementations to specs-go so that we can mark these as deprecated. Something like:

// ToOCI returns the opencontainers runtime Spec Hook for this Hook.
// 
// Deprecated: Use cdi.Hook.ToOCI instead.
func (h *Hook) ToOCI() interface{} {

}

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to keep these APIs in the specs-go and add deprecation warnings to them, so both old and new APIs would co-exist for a deprecation period?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue with that is that it would not remove the dependency, but yes, we could do that. Would you have a recommendation as to what the deprecation period should be?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if we really deprecate this the right way, we don't get rid of the dependency yet (so we don't return an interface either as suggested). The other alternative would be to do it the 'incorrectly/agressively', and because of the suggested empty interface-return signature change I am not sure if that is what @bart0sh was after, to keep a dummy implementation that is marked as deprecated, and maybe even panic from it.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for doing it aggressively. It makes sense after the above explanation, thanks @klihub


import (
spec "github.com/opencontainers/runtime-spec/specs-go"
)

// toOCI returns the opencontainers runtime Spec Hook for this Hook.
func (h *Hook) toOCI() spec.Hook {
return spec.Hook{
Path: h.Path,
Args: h.Args,
Env: h.Env,
Timeout: h.Timeout,
}
}

// toOCI returns the opencontainers runtime Spec Mount for this Mount.
func (m *Mount) toOCI() spec.Mount {
return spec.Mount{
Source: m.HostPath,
Destination: m.ContainerPath,
Options: m.Options,
Type: m.Type,
}
}

// toOCI returns the opencontainers runtime Spec LinuxDevice for this DeviceNode.
func (d *DeviceNode) toOCI() spec.LinuxDevice {
return spec.LinuxDevice{
Path: d.Path,
Type: d.Type,
Major: d.Major,
Minor: d.Minor,
FileMode: d.FileMode,
UID: d.UID,
GID: d.GID,
}
}

// toOCI returns the opencontainers runtime Spec LinuxIntelRdt for this IntelRdt config.
func (i *IntelRdt) toOCI() *spec.LinuxIntelRdt {
return &spec.LinuxIntelRdt{
ClosID: i.ClosID,
L3CacheSchema: i.L3CacheSchema,
MemBwSchema: i.MemBwSchema,
EnableCMT: i.EnableCMT,
EnableMBM: i.EnableMBM,
}
}
2 changes: 0 additions & 2 deletions specs-go/go.mod
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
module tags.cncf.io/container-device-interface/specs-go

go 1.19

require github.com/opencontainers/runtime-spec v1.1.0
2 changes: 0 additions & 2 deletions specs-go/go.sum
Original file line number Diff line number Diff line change
@@ -1,2 +0,0 @@
github.com/opencontainers/runtime-spec v1.1.0 h1:HHUyrt9mwHUjtasSbXSMvs4cyFxh+Bll4AjJ9odEGpg=
github.com/opencontainers/runtime-spec v1.1.0/go.mod h1:jwyrGlmzljRJv/Fgzds9SsS/C5hL+LL3ko9hs6T5lQ0=
77 changes: 42 additions & 35 deletions specs-go/oci.go
Original file line number Diff line number Diff line change
@@ -1,49 +1,56 @@
/*
Copyright © 2021 The CDI Authors

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package specs

import (
spec "github.com/opencontainers/runtime-spec/specs-go"
)
import "errors"

// errDeprecated is returned for the ToOCI functions below.
// This should provide better guidance for user when migrating from the API
// below to the APIs provided in the cdi package.
var errDeprecated = errors.New("deprecated; Use cdi package functions instead")

// ToOCI returns the opencontainers runtime Spec Hook for this Hook.
func (h *Hook) ToOCI() spec.Hook {
return spec.Hook{
Path: h.Path,
Args: h.Args,
Env: h.Env,
Timeout: h.Timeout,
}
//
// Deprecated: This function has been moved to tags.cncf.io/container-device-interface/pkg/cdi.Hook.toOCI
// and made private.
func (h *Hook) ToOCI() error {
return errDeprecated
}

// ToOCI returns the opencontainers runtime Spec Mount for this Mount.
func (m *Mount) ToOCI() spec.Mount {
return spec.Mount{
Source: m.HostPath,
Destination: m.ContainerPath,
Options: m.Options,
Type: m.Type,
}
//
// Deprecated: This function has been moved to tags.cncf.io/container-device-interface/pkg/cdi.Mount.toOCI
// and made private.
func (m *Mount) ToOCI() error {
return errDeprecated
}

// ToOCI returns the opencontainers runtime Spec LinuxDevice for this DeviceNode.
func (d *DeviceNode) ToOCI() spec.LinuxDevice {
return spec.LinuxDevice{
Path: d.Path,
Type: d.Type,
Major: d.Major,
Minor: d.Minor,
FileMode: d.FileMode,
UID: d.UID,
GID: d.GID,
}
//
// Deprecated: This function has been moved to tags.cncf.io/container-device-interface/pkg/cdi.DeviceNode.toOCI
// and made private.
func (d *DeviceNode) ToOCI() error {
return errDeprecated
}

// ToOCI returns the opencontainers runtime Spec LinuxIntelRdt for this IntelRdt config.
func (i *IntelRdt) ToOCI() *spec.LinuxIntelRdt {
return &spec.LinuxIntelRdt{
ClosID: i.ClosID,
L3CacheSchema: i.L3CacheSchema,
MemBwSchema: i.MemBwSchema,
EnableCMT: i.EnableCMT,
EnableMBM: i.EnableMBM,
}
//
// Deprecated: This function has been moved to tags.cncf.io/container-device-interface/pkg/cdi.IntelRdt.toOCI
// and made private.
func (i *IntelRdt) ToOCI() error {
return errDeprecated
}