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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
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
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

}
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