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

providers/proxmoxve: Add Proxmox VE provider #1790

Closed
wants to merge 1 commit into from
Closed
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
130 changes: 130 additions & 0 deletions internal/providers/proxmoxve/proxmoxve.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// Copyright 2019 Red Hat, Inc.
//
// 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.

// The OpenStack provider fetches configurations from the userdata available in
// both the config-drive as well as the network metadata service. Whichever
// responds first is the config that is used.
// NOTE: This provider is still EXPERIMENTAL.
Comment on lines +15 to +18
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is accurate. It might come from copy-paste?

Choose a reason for hiding this comment

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

@arcln can you have a quick look ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was the one who copy-pasted this, and my rationale for not updating the comment was that the same vestigial OpenStack comment lives on inside internal/providers/ibmcloud/ibmcloud.go

I probably should have updated it. I'm pretty sure this Proxmox provider doesn't attempt to look for a network server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, @b- legal stuff is always a bit spooky, from my reading I believe that keeping the original licensing note to be accurate.


package proxmoxve

import (
"context"
"fmt"
"os"
"os/exec"
"path/filepath"
"time"

"github.com/coreos/ignition/v2/config/v3_5_experimental/types"
"github.com/coreos/ignition/v2/internal/distro"
"github.com/coreos/ignition/v2/internal/log"
"github.com/coreos/ignition/v2/internal/platform"
"github.com/coreos/ignition/v2/internal/providers/util"
"github.com/coreos/ignition/v2/internal/resource"
ut "github.com/coreos/ignition/v2/internal/util"

"github.com/coreos/vcontext/report"
)

const (
cidataPath = "/user-data"
deviceLabel = "cidata"
)

func init() {
platform.Register(
platform.Provider{
Name: "proxmoxve",
Fetch: fetchConfig,
},
)
}

func fetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) {
var data []byte
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)

dispatch := func(name string, fn func() ([]byte, error)) {
raw, err := fn()
if err != nil {
switch err {
case context.Canceled:
case context.DeadlineExceeded:
f.Logger.Err("timed out while fetching config from %s", name)
default:
f.Logger.Err("failed to fetch config from %s: %v", name, err)
}
return
}

data = raw
cancel()
}

go dispatch(
"config drive (cidata)", func() ([]byte, error) {
return fetchConfigFromDevice(f.Logger, ctx, filepath.Join(distro.DiskByLabelDir(), deviceLabel))
},
)

<-ctx.Done()
if ctx.Err() == context.DeadlineExceeded {
f.Logger.Info("cidata drive was not available in time. Continuing without a config...")
}
Comment on lines +59 to +85
Copy link
Member

Choose a reason for hiding this comment

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

This construction is not desirable if we can avoid it. It's currently only used on platforms where it's possible for the drive to not exist and so we have to time out. The core problem is that it's inherently racy to wait around for a drive to show up. So if we know that there will always be a drive, then we should just indefinitely wait for it.

fetchConfigFromDevice() is already looping waiting for the device to show up so we can call it directly from here without using a context (and get rid of the context and select there). See e.g. the powervs, nutanix, or kubevirt provider code for examples of this pattern.

Or: is it the case that on Proxmox VE, no config drive may be attached? (Doesn't seem like it from my understanding.)

Choose a reason for hiding this comment

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

I confirm that it is possible not to attach a drive. It actually is the default behavior.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I don't follow. If Proxmox uses the drive the pass its own metadata, doesn't it need to provide it even if the user didn't provide any user-data? Or are you saying if users use the lower-level APIs it's possible?

But anyway, that's enough to require a timeout in that case.

Choose a reason for hiding this comment

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

Let me clarify, you can face :

  1. no drive at all attached to the guest VM
  2. a drive containing cloud-config as user data
  3. a drive containing ignition json as user data


return util.ParseConfig(f.Logger, data)
}

func fileExists(path string) bool {
_, err := os.Stat(path)
return (err == nil)
}

func fetchConfigFromDevice(logger *log.Logger, ctx context.Context, path string) ([]byte, error) {
for !fileExists(path) {
logger.Debug("config drive (%q) not found. Waiting...", path)
select {
case <-time.After(time.Second):
case <-ctx.Done():
return nil, ctx.Err()
}
}

logger.Debug("creating temporary mount point")
mnt, err := os.MkdirTemp("", "ignition-configdrive")
if err != nil {
return nil, fmt.Errorf("failed to create temp directory: %v", err)
}
defer os.Remove(mnt)

cmd := exec.Command(distro.MountCmd(), "-o", "ro", "-t", "auto", path, mnt)
if _, err := logger.LogCmd(cmd, "mounting config drive"); err != nil {
return nil, err
}
defer func() {
_ = logger.LogOp(
func() error {
return ut.UmountPath(mnt)
},
"unmounting %q at %q", path, mnt,
)
}()

if !fileExists(filepath.Join(mnt, cidataPath)) {
return nil, nil
}

return os.ReadFile(filepath.Join(mnt, cidataPath))
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to implement the "if the user-data is a cloud-config, then ignore it" here?

Choose a reason for hiding this comment

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

@arcln would have a better memory than mine, but I think we did implement this in another PR and that it was considered useless as ignition does verify if the content is a valid ignition configuration.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this can work. ParseConfig will error out because it's not a valid Ignition config and Ignition will exit nonzero, which will break boot.

In the other PR, it looks like we tried to workaround this by just ignoring errors completely, but that doesn't seem right either. We should explicitly check if the first line is #cloud-config, if yes, gracefully ignore, if no, then try to parse as an Ignition config and fail if it's not.

Choose a reason for hiding this comment

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

Again, I am not sure about this, let's wait for @arcln feedback

Copy link
Contributor

Choose a reason for hiding this comment

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

According to @pothos, this check is performed by Flatcar itself. Ignition will not be invoked if the user data file starts with #cloud-config. I don't know where precisely this check is performed, but I tried to boot the test Flatcar image with a cloud-config and it booted successfully.

I don't know if the same is true for FCOS. Can someone confirm ?

Copy link
Member

Choose a reason for hiding this comment

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

There's nothing in FCOS that would check if the user-data is a cloud-config and skip Ignition if so.

Choose a reason for hiding this comment

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

ok @jlebon so what do you think we should do ?
Implement a configuration check in ignition ?
If so, should it generically check a valid json or a discrete cloud config file type or header ?
Should this be implemented in this PR and in the ProxmoxVE part of the code or something reusable ?

Copy link
Member

Choose a reason for hiding this comment

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

ok @jlebon so what do you think we should do ? Implement a configuration check in ignition ? If so, should it generically check a valid json or a discrete cloud config file type or header ? Should this be implemented in this PR and in the ProxmoxVE part of the code or something reusable ?

I think to start we should be as strict as possible, i.e. only ignore quietly if the document starts with #cloud-config\n. If it doesn't, just assume it's JSON and try to parse it. I would keep it constrained to the Proxmox VE provider code.

}
1 change: 1 addition & 0 deletions internal/register/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import (
_ "github.com/coreos/ignition/v2/internal/providers/openstack"
_ "github.com/coreos/ignition/v2/internal/providers/packet"
_ "github.com/coreos/ignition/v2/internal/providers/powervs"
_ "github.com/coreos/ignition/v2/internal/providers/proxmoxve"
_ "github.com/coreos/ignition/v2/internal/providers/qemu"
_ "github.com/coreos/ignition/v2/internal/providers/scaleway"
_ "github.com/coreos/ignition/v2/internal/providers/virtualbox"
Expand Down
Loading