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

feat: Warn user of unexpected run mode #5209

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
14 changes: 10 additions & 4 deletions cloudinit/cmd/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,21 @@ def print_exc(msg=""):
sys.stderr.write("\n")


def log_ppid():
if util.is_Linux():
def log_ppid(distro):
if distro.is_linux:
ppid = os.getppid()
LOG.info("PID [%s] started cloud-init.", ppid)
log = LOG.info
extra_message = ""
if 1 != ppid and distro.uses_systemd():
log = LOG.warning
extra_message = ("Not a supported configuration.",)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add details in the extra message to tell use what's unsupported about it.
Unsupported configuration: boot stage called outside of systemd

log("PID [%s] started cloud-init. %s", ppid, extra_message)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we intentionally not logging the PID when not distro.is_linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what the code did before, but I suppose we should be able to log PPID on non-linux too. I'm curious whether the PPID=1 should hold true on any of the BSDs, and it wouldn't hurt to log it.



def welcome(action, msg=None):
if not msg:
msg = welcome_format(action)
util.multi_log("%s\n" % (msg), console=False, stderr=True, log=LOG)
log_ppid()
return msg


Expand Down Expand Up @@ -324,6 +328,7 @@ def main_init(name, args):
init = stages.Init(ds_deps=deps, reporter=args.reporter)
# Stage 1
init.read_cfg(extract_fns(args))
log_ppid(init.distro)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This cannot happen until after log.setup_logging has been called.

# Stage 2
outfmt = None
errfmt = None
Expand Down Expand Up @@ -580,6 +585,7 @@ def main_modules(action_name, args):
init = stages.Init(ds_deps=[], reporter=args.reporter)
# Stage 1
init.read_cfg(extract_fns(args))
log_ppid(init.distro)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to move this down a bit too after log.setup_logging otherwise no logs are written to /var/log/cloud-init.log. To retain original behavior, we may want to log this just after the welcome(... w_msg) to log it during the boot stage block.

# Stage 2
try:
init.fetch(existing="trust")
Expand Down
3 changes: 3 additions & 0 deletions cloudinit/distros/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ def __init__(self, name, cfg, paths):
self.package_managers: List[PackageManager] = []
self._dhcp_client = None
self._fallback_interface = None
self.is_linux = True

def _unpickle(self, ci_pkl_version: int) -> None:
"""Perform deserialization fixes for Distro."""
Expand All @@ -185,6 +186,8 @@ def _unpickle(self, ci_pkl_version: int) -> None:
self._dhcp_client = None
if not hasattr(self, "_fallback_interface"):
self._fallback_interface = None
if not hasattr(self, "is_linux"):
self.is_linux = True

def _validate_entry(self, entry):
if isinstance(entry, str):
Expand Down
7 changes: 7 additions & 0 deletions cloudinit/distros/bsd.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ def __init__(self, name, cfg, paths):
cfg["rsyslog_svcname"] = "rsyslogd"
self.osfamily = platform.system().lower()
self.net_ops = bsd_netops.BsdNetOps
self.is_linux = False

def _unpickle(self, ci_pkl_version: int) -> None:
super()._unpickle(ci_pkl_version)

# this needs to be after the super class _unpickle to override it
self.is_linux = False

def _read_system_hostname(self):
sys_hostname = self._read_hostname(self.hostname_conf_fn)
Expand Down
6 changes: 6 additions & 0 deletions cloudinit/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,12 @@ def multi_log(

@lru_cache()
def is_Linux():
"""deprecated: prefer Distro object's `is_linux` property
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could just drop this function altogether if we can figure a better approach to sort cloud-init analyze dump as the only other use-case right?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep!


Multiple sources of truth is bad, and already know whether we are
working with Linux from the Distro class. Using Distro offers greater code
reusablity, cleaner code, and easier maintenance.
"""
return "Linux" in platform.system()


Expand Down