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.

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

Jump to bottom

MGMT-15150: Use same installer binary for all platform types #5334

Merged
merged 2 commits into from Jul 9, 2023
Merged

MGMT-15150: Use same installer binary for all platform types #5334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c05bccb
MGMT-15150: Use same installer binary for all platform types
zaneb Jul 3, 2023
e73b9c5
Remove platform argument when extracting installer
zaneb Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions internal/bminventory/inventory.go
View file
Open in desktop
Expand Up @@ -6443,15 +6443,14 @@ func (b *bareMetalInventory) GetKnownApprovedHosts(clusterId strfmt.UUID) ([]*co
return b.hostApi.GetKnownApprovedHosts(clusterId)
}

// In case cpu architecture is not x86_64 and platform is baremetal, we should extract openshift-baremetal-installer
// In case cpu architecture is not x86_64, we should extract openshift-baremetal-installer
// from x86_64 release image as there is no x86_64 openshift-baremetal-installer executable in arm image.
// This flow does not affect the multiarch release images and is meant purely for using arm64 release image with the x86 hub.
// Implementation of handling the multiarch images is done directly in the `oc` binary and relies on the fact that `oc adm release extract`
// will automatically use the image matching the Hub's architecture.
func isBaremetalBinaryFromAnotherReleaseImageRequired(cpuArchitecture, version string, platform *models.PlatformType) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the func might be a bit confusing now... Maybe worth adding a comment about none platform clusters (i.e. that also the baremetal binary variant can be used).

Copy link
Member Author

Choose a reason for hiding this comment

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

It's still the -baremetal- binary, so I think the name is still accurate.
TBH it might be less confusing than before, where the logic here said platform == baremetal and in the other location platform != none, and we were relying on the fact that no other platforms are supported on different architectures.

return cpuArchitecture != common.MultiCPUArchitecture &&
cpuArchitecture != common.NormalizeCPUArchitecture(runtime.GOARCH) &&
common.PlatformTypeValue(platform) == models.PlatformTypeBaremetal &&
featuresupport.IsFeatureAvailable(models.FeatureSupportLevelIDCLUSTERMANAGEDNETWORKING, version, swag.String(models.ClusterCPUArchitectureArm64))
}

Expand Down
3 changes: 1 addition & 2 deletions internal/ignition/dummy.go
View file
Open in desktop
Expand Up @@ -7,7 +7,6 @@ import (

"github.com/openshift/assisted-service/internal/common"
"github.com/openshift/assisted-service/internal/host/hostutil"
"github.com/openshift/assisted-service/models"
"github.com/openshift/assisted-service/pkg/auth"
"github.com/openshift/assisted-service/pkg/s3wrapper"
"github.com/sirupsen/logrus"
Expand All @@ -33,7 +32,7 @@ func NewDummyGenerator(serviceBaseURL string, workDir string, cluster *common.Cl
}

// Generate creates the expected ignition and related files but with nonsense content
func (g *dummyGenerator) Generate(_ context.Context, installConfig []byte, platformType models.PlatformType, authType auth.AuthType) error {
func (g *dummyGenerator) Generate(_ context.Context, installConfig []byte, authType auth.AuthType) error {
toUpload := fileNames[:]
for _, host := range g.cluster.Hosts {
toUpload = append(toUpload, hostutil.IgnitionFileName(host))
Expand Down
6 changes: 3 additions & 3 deletions internal/ignition/ignition.go
View file
Open in desktop
Expand Up @@ -202,7 +202,7 @@ var fileNames = [...]string{

// Generator can generate ignition files and upload them to an S3-like service
type Generator interface {
Generate(ctx context.Context, installConfig []byte, platformType models.PlatformType, authType auth.AuthType) error
Generate(ctx context.Context, installConfig []byte, authType auth.AuthType) error
UploadToS3(ctx context.Context) error
UpdateEtcHosts(string) error
}
Expand Down Expand Up @@ -307,7 +307,7 @@ func (g *installerGenerator) UploadToS3(ctx context.Context) error {
}

// Generate generates ignition files and applies modifications.
func (g *installerGenerator) Generate(ctx context.Context, installConfig []byte, platformType models.PlatformType, authType auth.AuthType) error {
func (g *installerGenerator) Generate(ctx context.Context, installConfig []byte, authType auth.AuthType) error {
var err error
log := logutil.FromContext(ctx, g.log)

Expand All @@ -328,7 +328,7 @@ func (g *installerGenerator) Generate(ctx context.Context, installConfig []byte,
MaxTries: oc.DefaultTries, RetryDelay: oc.DefaltRetryDelay}, mirrorRegistriesBuilder)

release, err := g.installerCache.Get(g.installerReleaseImageOverride, g.releaseImageMirror,
g.cluster.PullSecret, ocRelease, platformType)
g.cluster.PullSecret, ocRelease)
if err != nil {
return errors.Wrap(err, "failed to get installer path")
}
Expand Down
8 changes: 4 additions & 4 deletions internal/ignition/mock_ignition.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 3 additions & 4 deletions internal/installercache/installercache.go
View file
Open in desktop
Expand Up @@ -10,7 +10,6 @@ import (
"time"

"github.com/openshift/assisted-service/internal/oc"
"github.com/openshift/assisted-service/models"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)
Expand Down Expand Up @@ -64,7 +63,7 @@ func New(cacheDir string, storageCapacity int64, log logrus.FieldLogger) *Instal
// Get returns the path to an openshift-baremetal-install binary extracted from
// the referenced release image. Tries the mirror release image first if it's set. It is safe for concurrent use. A cache of
// binaries is maintained to reduce re-downloading of the same release.
func (i *Installers) Get(releaseID, releaseIDMirror, pullSecret string, ocRelease oc.Release, platformType models.PlatformType) (*Release, error) {
func (i *Installers) Get(releaseID, releaseIDMirror, pullSecret string, ocRelease oc.Release) (*Release, error) {
i.Lock()
defer i.Unlock()

Expand All @@ -75,13 +74,13 @@ func (i *Installers) Get(releaseID, releaseIDMirror, pullSecret string, ocReleas
if releaseIDMirror != "" {
releaseImageLocation = releaseIDMirror
}
workdir, binary, path = ocRelease.GetReleaseBinaryPath(releaseImageLocation, i.cacheDir, platformType)
workdir, binary, path = ocRelease.GetReleaseBinaryPath(releaseImageLocation, i.cacheDir)
if _, err = os.Stat(path); os.IsNotExist(err) {
//evict older files if necessary
i.evict()

//extract the binary
_, err = ocRelease.Extract(i.log, releaseID, releaseIDMirror, i.cacheDir, pullSecret, platformType)
_, err = ocRelease.Extract(i.log, releaseID, releaseIDMirror, i.cacheDir, pullSecret)
if err != nil {
return &Release{}, err
}
Expand Down
28 changes: 12 additions & 16 deletions internal/installercache/installercache_test.go
View file
Open in desktop
Expand Up @@ -11,7 +11,6 @@ import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"
"github.com/openshift/assisted-service/internal/oc"
"github.com/openshift/assisted-service/models"
"github.com/sirupsen/logrus"
)

Expand Down Expand Up @@ -46,16 +45,15 @@ var _ = Describe("installer cache", func() {
fname := filepath.Join(workdir, releaseID)

mockRelease.EXPECT().GetReleaseBinaryPath(
gomock.Any(), gomock.Any(), gomock.Any()).
gomock.Any(), gomock.Any()).
Return(workdir, releaseID, fname)
mockRelease.EXPECT().Extract(gomock.Any(), releaseID,
gomock.Any(), cacheDir, gomock.Any(),
gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string, platformType models.PlatformType) (string, error) {
gomock.Any(), cacheDir, gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string) (string, error) {
err := os.WriteFile(fname, []byte("abcde"), 0600)
return "", err
})
l, err := manager.Get(releaseID, "mirror", "pull-secret", mockRelease, models.PlatformTypeBaremetal)
l, err := manager.Get(releaseID, "mirror", "pull-secret", mockRelease)
Expect(err).ShouldNot(HaveOccurred())

time.Sleep(1 * time.Second)
Expand Down Expand Up @@ -123,16 +121,15 @@ var _ = Describe("installer cache", func() {
fname := filepath.Join(workdir, releaseID)

mockRelease.EXPECT().GetReleaseBinaryPath(
releaseMirrorID, gomock.Any(), gomock.Any()).
releaseMirrorID, gomock.Any()).
Return(workdir, releaseID, fname)
mockRelease.EXPECT().Extract(gomock.Any(), releaseID,
gomock.Any(), cacheDir, gomock.Any(),
gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string, platformType models.PlatformType) (string, error) {
gomock.Any(), cacheDir, gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string) (string, error) {
err := os.WriteFile(fname, []byte("abcde"), 0600)
return "", err
})
_, err := manager.Get(releaseID, releaseMirrorID, "pull-secret", mockRelease, models.PlatformTypeBaremetal)
_, err := manager.Get(releaseID, releaseMirrorID, "pull-secret", mockRelease)
Expect(err).ShouldNot(HaveOccurred())
})

Expand All @@ -143,16 +140,15 @@ var _ = Describe("installer cache", func() {
fname := filepath.Join(workdir, releaseID)

mockRelease.EXPECT().GetReleaseBinaryPath(
releaseID, gomock.Any(), gomock.Any()).
releaseID, gomock.Any()).
Return(workdir, releaseID, fname)
mockRelease.EXPECT().Extract(gomock.Any(), releaseID,
gomock.Any(), cacheDir, gomock.Any(),
gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string, platformType models.PlatformType) (string, error) {
gomock.Any(), cacheDir, gomock.Any()).
DoAndReturn(func(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string) (string, error) {
err := os.WriteFile(fname, []byte("abcde"), 0600)
return "", err
})
_, err := manager.Get(releaseID, releaseMirrorID, "pull-secret", mockRelease, models.PlatformTypeBaremetal)
_, err := manager.Get(releaseID, releaseMirrorID, "pull-secret", mockRelease)
Expect(err).ShouldNot(HaveOccurred())
})
})
Expand Down
17 changes: 8 additions & 9 deletions internal/oc/mock_release.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 9 additions & 17 deletions internal/oc/release.go
View file
Open in desktop
Expand Up @@ -13,7 +13,6 @@ import (
"github.com/hashicorp/go-version"
operatorv1alpha1 "github.com/openshift/api/operator/v1alpha1"
"github.com/openshift/assisted-service/internal/common"
"github.com/openshift/assisted-service/models"
"github.com/openshift/assisted-service/pkg/executer"
"github.com/openshift/assisted-service/pkg/mirrorregistries"
"github.com/patrickmn/go-cache"
Expand Down Expand Up @@ -47,8 +46,8 @@ type Release interface {
GetOpenshiftVersion(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, pullSecret string) (string, error)
GetMajorMinorVersion(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, pullSecret string) (string, error)
GetReleaseArchitecture(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, pullSecret string) ([]string, error)
GetReleaseBinaryPath(releaseImage string, cacheDir string, platformType models.PlatformType) (workdir string, binary string, path string)
Extract(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string, platformType models.PlatformType) (string, error)
GetReleaseBinaryPath(releaseImage string, cacheDir string) (workdir string, binary string, path string)
Extract(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string) (string, error)
}

type imageValue struct {
Expand Down Expand Up @@ -314,7 +313,7 @@ func (r *release) getOpenshiftVersionFromRelease(log logrus.FieldLogger, release

// Extract openshift-baremetal-install binary from releaseImageMirror if provided.
// Else extract from the source releaseImage
func (r *release) Extract(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string, platformType models.PlatformType) (string, error) {
func (r *release) Extract(log logrus.FieldLogger, releaseImage string, releaseImageMirror string, cacheDir string, pullSecret string) (string, error) {
var path string
var err error
if releaseImage == "" && releaseImageMirror == "" {
Expand All @@ -329,13 +328,13 @@ func (r *release) Extract(log logrus.FieldLogger, releaseImage string, releaseIm

if releaseImageMirror != "" {
//TODO: Get mirror registry certificate from install-config
path, err = r.extractFromRelease(log, releaseImageMirror, cacheDir, pullSecret, true, platformType, icspFile)
path, err = r.extractFromRelease(log, releaseImageMirror, cacheDir, pullSecret, true, icspFile)
if err != nil {
log.WithError(err).Errorf("failed to extract openshift-baremetal-install from mirror release image %s", releaseImageMirror)
return "", err
}
} else {
path, err = r.extractFromRelease(log, releaseImage, cacheDir, pullSecret, false, platformType, icspFile)
path, err = r.extractFromRelease(log, releaseImage, cacheDir, pullSecret, false, icspFile)
if err != nil {
log.WithError(err).Errorf("failed to extract openshift-baremetal-install from release image %s", releaseImage)
return "", err
Expand All @@ -344,24 +343,17 @@ func (r *release) Extract(log logrus.FieldLogger, releaseImage string, releaseIm
return path, err
}

func (r *release) GetReleaseBinaryPath(releaseImage string, cacheDir string, platformType models.PlatformType) (workdir string, binary string, path string) {
// Using platform type as an indication for which openshift install binary to use
// (e.g. as non-x86_64 clusters should use the openshift-install binary).
if platformType == models.PlatformTypeNone {
binary = "openshift-install"
} else {
binary = "openshift-baremetal-install"
}

func (r *release) GetReleaseBinaryPath(releaseImage string, cacheDir string) (workdir string, binary string, path string) {
workdir = filepath.Join(cacheDir, releaseImage)
binary = "openshift-baremetal-install"
path = filepath.Join(workdir, binary)
return
}

// extractFromRelease returns the path to an openshift-baremetal-install binary extracted from
// the referenced release image.
func (r *release) extractFromRelease(log logrus.FieldLogger, releaseImage, cacheDir, pullSecret string, insecure bool, platformType models.PlatformType, icspFile string) (string, error) {
workdir, binary, path := r.GetReleaseBinaryPath(releaseImage, cacheDir, platformType)
func (r *release) extractFromRelease(log logrus.FieldLogger, releaseImage, cacheDir, pullSecret string, insecure bool, icspFile string) (string, error) {
workdir, binary, path := r.GetReleaseBinaryPath(releaseImage, cacheDir)
log.Infof("extracting %s binary to %s", binary, workdir)
err := os.MkdirAll(workdir, 0755)
if err != nil {
Expand Down