Skip to content

Commit

Permalink
manifests: add NewWithOptions
Browse files Browse the repository at this point in the history 
pkg/manifests/rte/rte.go GetManifests() gathered a fair number of
parameters, with little hope to go down.
Add a new API which packs them in a struct to improve readability.

Compare:
```
mf, err := GetManifests(tc.plat, tc.platVersion, "test", true, tc.withCustomSELinuxPolicy)
```

With:
```
mf, err := NewWithOptions(options.Render{
	Platform:            tc.plat,
	PlatformVersion:     tc.platVersion,
	Namespace:           "test",
	EnableCRIHooks:      true,
	CustomSELinuxPolicy: tc.withCustomSELinuxPolicy,
})
```

note GetManifests is preserved, but deprecated.
NewWithOptions is the preferred API.

Signed-off-by: Francesco Romani <[email protected]>
  • Loading branch information
@ffromani
ffromani committed Dec 11, 2024
1 parent 9ae9c23 commit b347613
Show file tree
Hide file tree
Showing 18 changed files with 308 additions and 73 deletions.
22 changes: 16 additions & 6 deletions pkg/commands/render.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import (
"github.com/k8stopologyawareschedwg/deployer/pkg/deployer/platform"
"github.com/k8stopologyawareschedwg/deployer/pkg/deployer/updaters"
"github.com/k8stopologyawareschedwg/deployer/pkg/manifests"
"github.com/k8stopologyawareschedwg/deployer/pkg/manifests/api"
"github.com/k8stopologyawareschedwg/deployer/pkg/manifests/sched"
apimanifests "github.com/k8stopologyawareschedwg/deployer/pkg/manifests/api"
schedmanifests "github.com/k8stopologyawareschedwg/deployer/pkg/manifests/sched"
"github.com/k8stopologyawareschedwg/deployer/pkg/options"
)

Expand Down Expand Up @@ -61,7 +61,9 @@ func NewRenderAPICommand(env *deployer.Environment, commonOpts *options.Options,
if commonOpts.UserPlatform == platform.Unknown {
return fmt.Errorf("must explicitly select a cluster platform")
}
apiManifests, err := api.GetManifests(commonOpts.UserPlatform)
apiManifests, err := apimanifests.NewWithOptions(options.Render{
Platform: commonOpts.UserPlatform,
})
if err != nil {
return err
}
Expand Down Expand Up @@ -90,7 +92,10 @@ func NewRenderSchedulerPluginCommand(env *deployer.Environment, commonOpts *opti
return err
}

schedManifests, err := sched.GetManifests(commonOpts.UserPlatform, namespace)
schedManifests, err := schedmanifests.NewWithOptions(options.Render{
Platform: commonOpts.UserPlatform,
Namespace: namespace,
})
if err != nil {
return err
}
Expand Down Expand Up @@ -159,7 +164,9 @@ func makeUpdaterObjects(commonOpts *options.Options) ([]client.Object, string, e
func RenderManifests(env *deployer.Environment, commonOpts *options.Options) error {
var objs []client.Object

apiManifests, err := api.GetManifests(commonOpts.UserPlatform)
apiManifests, err := apimanifests.NewWithOptions(options.Render{
Platform: commonOpts.UserPlatform,
})
if err != nil {
return err
}
Expand All @@ -175,7 +182,10 @@ func RenderManifests(env *deployer.Environment, commonOpts *options.Options) err
}
objs = append(objs, updaterObjs...)

schedManifests, err := sched.GetManifests(commonOpts.UserPlatform, updaterNs)
schedManifests, err := schedmanifests.NewWithOptions(options.Render{
Platform: commonOpts.UserPlatform,
Namespace: updaterNs,
})
if err != nil {
return err
}
Expand Down
8 changes: 6 additions & 2 deletions pkg/deployer/api/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ func Deploy(env *deployer.Environment, opts options.API) error {
env = env.WithName("API")
env.Log.Info("deploying topology-aware-scheduling API")

mf, err := apimanifests.GetManifests(opts.Platform)
mf, err := apimanifests.NewWithOptions(options.Render{
Platform: opts.Platform,
})
if err != nil {
return err
}
Expand Down Expand Up @@ -67,7 +69,9 @@ func Remove(env *deployer.Environment, opts options.API) error {
env = env.WithName("API")
env.Log.Info("removing topology-aware-scheduling API")

mf, err := apimanifests.GetManifests(opts.Platform)
mf, err := apimanifests.NewWithOptions(options.Render{
Platform: opts.Platform,
})
if err != nil {
return err
}
Expand Down
8 changes: 6 additions & 2 deletions pkg/deployer/sched/sched.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ func Deploy(env *deployer.Environment, opts options.Scheduler) error {
env = env.WithName("SCD")
env.Log.Info("deploying topology-aware-scheduling scheduler plugin")

mf, err := schedmanifests.GetManifests(opts.Platform, "")
mf, err := schedmanifests.NewWithOptions(options.Render{
Platform: opts.Platform,
})
if err != nil {
return err
}
Expand Down Expand Up @@ -72,7 +74,9 @@ func Remove(env *deployer.Environment, opts options.Scheduler) error {
env = env.WithName("SCD")
env.Log.Info("removing topology-aware-scheduling scheduler plugin")

mf, err := schedmanifests.GetManifests(opts.Platform, "")
mf, err := schedmanifests.NewWithOptions(options.Render{
Platform: opts.Platform,
})
if err != nil {
return err
}
Expand Down
22 changes: 16 additions & 6 deletions pkg/deployer/updaters/objects.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import (

func GetObjects(opts options.Updater, updaterType, namespace string) ([]client.Object, error) {
if updaterType == RTE {
mf, err := rtemanifests.GetManifests(opts.Platform, opts.PlatformVersion, namespace, opts.EnableCRIHooks, opts.CustomSELinuxPolicy)
mf, err := rtemanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -43,7 +43,7 @@ func GetObjects(opts options.Updater, updaterType, namespace string) ([]client.O
return ret.ToObjects(), nil
}
if updaterType == NFD {
mf, err := nfdmanifests.GetManifests(opts.Platform, namespace)
mf, err := nfdmanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -58,7 +58,7 @@ func GetObjects(opts options.Updater, updaterType, namespace string) ([]client.O

func getCreatableObjects(env *deployer.Environment, opts options.Updater, updaterType, namespace string) ([]objectwait.WaitableObject, error) {
if updaterType == RTE {
mf, err := rtemanifests.GetManifests(opts.Platform, opts.PlatformVersion, namespace, opts.EnableCRIHooks, opts.CustomSELinuxPolicy)
mf, err := rtemanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -69,7 +69,7 @@ func getCreatableObjects(env *deployer.Environment, opts options.Updater, update
return rtewait.Creatable(ret, env.Cli, env.Log), nil
}
if updaterType == NFD {
mf, err := nfdmanifests.GetManifests(opts.Platform, namespace)
mf, err := nfdmanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -84,7 +84,7 @@ func getCreatableObjects(env *deployer.Environment, opts options.Updater, update

func getDeletableObjects(env *deployer.Environment, opts options.Updater, updaterType, namespace string) ([]objectwait.WaitableObject, error) {
if updaterType == RTE {
mf, err := rtemanifests.GetManifests(opts.Platform, opts.PlatformVersion, namespace, opts.EnableCRIHooks, opts.CustomSELinuxPolicy)
mf, err := rtemanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -95,7 +95,7 @@ func getDeletableObjects(env *deployer.Environment, opts options.Updater, update
return rtewait.Deletable(ret, env.Cli, env.Log), nil
}
if updaterType == NFD {
mf, err := nfdmanifests.GetManifests(opts.Platform, namespace)
mf, err := nfdmanifests.NewWithOptions(renderOptionsFrom(opts, namespace))
if err != nil {
return nil, err
}
Expand All @@ -115,3 +115,13 @@ func updaterDaemonOptionsFrom(opts options.Updater, namespace string) options.Up
Namespace: namespace,
}
}

func renderOptionsFrom(opts options.Updater, namespace string) options.Render {
return options.Render{
Platform: opts.Platform,
PlatformVersion: opts.PlatformVersion,
Namespace: namespace,
EnableCRIHooks: opts.EnableCRIHooks,
CustomSELinuxPolicy: opts.CustomSELinuxPolicy,
}
}
12 changes: 10 additions & 2 deletions pkg/manifests/api/api.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (

"github.com/k8stopologyawareschedwg/deployer/pkg/deployer/platform"
"github.com/k8stopologyawareschedwg/deployer/pkg/manifests"
"github.com/k8stopologyawareschedwg/deployer/pkg/options"
)

type Manifests struct {
Expand Down Expand Up @@ -57,9 +58,9 @@ func New(plat platform.Platform) Manifests {
}
}

func GetManifests(plat platform.Platform) (Manifests, error) {
func NewWithOptions(opts options.Render) (Manifests, error) {
var err error
mf := New(plat)
mf := New(opts.Platform)

mf.Crd, err = manifests.APICRD()
if err != nil {
Expand All @@ -68,3 +69,10 @@ func GetManifests(plat platform.Platform) (Manifests, error) {

return mf, nil
}

// GetManifests is deprecated, use NewWithOptions in new code
func GetManifests(plat platform.Platform) (Manifests, error) {
return NewWithOptions(options.Render{
Platform: plat,
})
}
19 changes: 13 additions & 6 deletions pkg/manifests/api/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"testing"

"github.com/k8stopologyawareschedwg/deployer/pkg/deployer/platform"
"github.com/k8stopologyawareschedwg/deployer/pkg/options"
)

func TestClone(t *testing.T) {
Expand All @@ -43,9 +44,11 @@ func TestClone(t *testing.T) {

for _, tc := range testCases {
var err error
tc.mf, err = GetManifests(tc.plat)
tc.mf, err = NewWithOptions(options.Render{
Platform: tc.plat,
})
if err != nil {
t.Fatalf("GetManifests() failed: %v", err)
t.Fatalf("NewWithOptions() failed: %v", err)
}

cMf := tc.mf.Clone()
Expand Down Expand Up @@ -75,9 +78,11 @@ func TestRender(t *testing.T) {

for _, tc := range testCases {
var err error
tc.mf, err = GetManifests(tc.plat)
tc.mf, err = NewWithOptions(options.Render{
Platform: tc.plat,
})
if err != nil {
t.Fatalf("GetManifests() failed: %v", err)
t.Fatalf("NewWithOptions() failed: %v", err)
}
mfBeforeRender := tc.mf.Clone()
uMf, err := tc.mf.Render()
Expand Down Expand Up @@ -114,9 +119,11 @@ func TestToObjects(t *testing.T) {

for _, tc := range testCases {
var err error
tc.mf, err = GetManifests(tc.plat)
tc.mf, err = NewWithOptions(options.Render{
Platform: tc.plat,
})
if err != nil {
t.Fatalf("GetManifests() failed: %v", err)
t.Fatalf("NewWithOptions() failed: %v", err)
}
objs := tc.mf.ToObjects()
if len(objs) == 0 {
Expand Down
16 changes: 12 additions & 4 deletions pkg/manifests/nfd/nfd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,16 +87,16 @@ func New(plat platform.Platform) Manifests {
return mf
}

func GetManifests(plat platform.Platform, namespace string) (Manifests, error) {
func NewWithOptions(opts options.Render) (Manifests, error) {
var err error
mf := New(plat)
mf := New(opts.Platform)

mf.Namespace, err = manifests.Namespace(manifests.ComponentNodeFeatureDiscovery)
if err != nil {
return mf, err
}

mf.SATopologyUpdater, err = manifests.ServiceAccount(manifests.ComponentNodeFeatureDiscovery, manifests.SubComponentNodeFeatureDiscoveryTopologyUpdater, namespace)
mf.SATopologyUpdater, err = manifests.ServiceAccount(manifests.ComponentNodeFeatureDiscovery, manifests.SubComponentNodeFeatureDiscoveryTopologyUpdater, opts.Namespace)
if err != nil {
return mf, err
}
Expand All @@ -108,10 +108,18 @@ func GetManifests(plat platform.Platform, namespace string) (Manifests, error) {
if err != nil {
return mf, err
}
mf.DSTopologyUpdater, err = manifests.DaemonSet(manifests.ComponentNodeFeatureDiscovery, manifests.SubComponentNodeFeatureDiscoveryTopologyUpdater, namespace)
mf.DSTopologyUpdater, err = manifests.DaemonSet(manifests.ComponentNodeFeatureDiscovery, manifests.SubComponentNodeFeatureDiscoveryTopologyUpdater, opts.Namespace)
if err != nil {
return mf, err
}

return mf, nil
}

// GetManifests is deprecated, use NewWithOptions in new code
func GetManifests(plat platform.Platform, namespace string) (Manifests, error) {
return NewWithOptions(options.Render{
Platform: plat,
Namespace: namespace,
})
}
19 changes: 13 additions & 6 deletions pkg/manifests/nfd/nfd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

const defaultNFDNamespace = "node-feature-discovery"

func TestGetManifests(t *testing.T) {
func TestNewWithOptions(t *testing.T) {
type testCase struct {
name string
namespace string
Expand All @@ -47,16 +47,19 @@ func TestGetManifests(t *testing.T) {
}

for _, tc := range testCases {
tc.mf, _ = GetManifests(tc.plat, tc.namespace)
tc.mf, _ = NewWithOptions(options.Render{
Platform: tc.plat,
Namespace: tc.namespace,
})
for _, obj := range tc.mf.ToObjects() {
if tc.namespace != "" {
if obj.GetNamespace() != "" && obj.GetNamespace() != tc.namespace {
t.Errorf("testcase %q, GetManifests failed to create %q object named %q with correct namespace %q. got namespace %q instead ",
t.Errorf("testcase %q, NewWithOptions failed to create %q object named %q with correct namespace %q. got namespace %q instead ",
tc.name, obj.GetObjectKind(), obj.GetName(), tc.namespace, obj.GetNamespace())
}
} else { // no namespace provided we should have the default
if obj.GetNamespace() != "" && obj.GetNamespace() != defaultNFDNamespace {
t.Errorf("testcase %q, GetManifests failed to create %q object named %q with default namespace %q. got namespace %q instead ",
t.Errorf("testcase %q, NewWithOptions failed to create %q object named %q with default namespace %q. got namespace %q instead ",
tc.name, obj.GetObjectKind(), obj.GetName(), defaultNFDNamespace, obj.GetNamespace())
}
}
Expand All @@ -82,7 +85,9 @@ func TestClone(t *testing.T) {
}

for _, tc := range testCases {
tc.mf, _ = GetManifests(tc.plat, "")
tc.mf, _ = NewWithOptions(options.Render{
Platform: tc.plat,
})
cMf := tc.mf.Clone()

if &cMf == &tc.mf {
Expand Down Expand Up @@ -110,7 +115,9 @@ func TestRender(t *testing.T) {
}

for _, tc := range testCases {
tc.mf, _ = GetManifests(tc.plat, "")
tc.mf, _ = NewWithOptions(options.Render{
Platform: tc.plat,
})
mfBeforeUpdate := tc.mf.Clone()
uMf, err := tc.mf.Render(options.UpdaterDaemon{})
if err != nil {
Expand Down
Loading

0 comments on commit b347613

Please sign in to comment.