[RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

When "make bringup" runs "terraform plan", it complains that the
kdevops_nodes variable isn't set.

extra_vars.yaml does contain the variable, but I'm guessing that
somehow gen_nodes runs after gen_tfvars now? Not really sure.

Fixes: 5457b742d611 ("Makefile: fix target dependency order")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index fb7e9b816dbc..c1dd1232c7ff 100644
--- a/Makefile
+++ b/Makefile
@@ -258,7 +258,7 @@ ifneq (,$(KDEVOPS_BRING_UP_DEPS))
 include scripts/bringup.Makefile
 endif
 
-$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
+$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_NODES) $(KDEVOPS_EXTRA_VARS)
 	$(Q)ANSIBLE_LOCALHOST_WARNING=False ANSIBLE_INVENTORY_UNPARSED_WARNING=False \
 		ansible-playbook $(ANSIBLE_VERBOSE) \
 		$(KDEVOPS_PLAYBOOKS_DIR)/gen_hosts.yml \
-- 
2.51.0

Re: [RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Daniel Gomez]

On 28/09/2025 17.53, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> When "make bringup" runs "terraform plan", it complains that the
> kdevops_nodes variable isn't set.
> 
> extra_vars.yaml does contain the variable, but I'm guessing that
> somehow gen_nodes runs after gen_tfvars now? Not really sure.

I see! Thanks for reporting. Indeed my fix introduced this bug. I think
it'd be best to move KDEVOPS_NODES just after we assign the filename to it
(provision.Makefile -> guestfs.Makefile):

diff --git a/Makefile b/Makefile
index 3653591d..26615b22 100644
--- a/Makefile
+++ b/Makefile
@@ -144,16 +144,16 @@ endif
 DEFAULT_DEPS += $(ANSIBLE_CFG_FILE)
 DEFAULT_DEPS += $(ANSIBLE_INVENTORY_FILE)

-ifneq (,$(KDEVOPS_NODES))
-DEFAULT_DEPS += $(KDEVOPS_NODES)
-endif
-
 include scripts/provision.Makefile
 include scripts/firstconfig.Makefile
 include scripts/systemd-timesync.Makefile
 include scripts/journal-server.Makefile
 include scripts/update_etc_hosts.Makefile

+ifneq (,$(KDEVOPS_NODES))
+DEFAULT_DEPS += $(KDEVOPS_NODES)
+endif
+
 KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
 KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_PROVISIONED_DEVCONFIG)

The patch below changes the proposed order (ansible.cfg -> hosts -> nodes ->
rest)  in patch 5457b742d611 to:

==> [/home/dagomez.linux/src/kdevops/ansible.cfg]
==> [guestfs/kdevops_nodes.yaml]
==> [/home/dagomez.linux/src/kdevops/hosts]

I think both approaches work because nodes do not seem to be requiring hosts.
The questions is whether we need both changes? I think we need to ensure 
nodes are consistent with inventory changes. And they other way around in 
case the user deletes any of the files. So, what do you think about adding
this on top of the above?

@@ -264,13 +264,13 @@ ifneq (,$(KDEVOPS_BRING_UP_DEPS))
 include scripts/bringup.Makefile
 endif

-$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
+$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_NODES) $(KDEVOPS_EXTRA_VARS)
        $(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
                --inventory localhost, \
                $(KDEVOPS_PLAYBOOKS_DIR)/gen_hosts.yml \
                --extra-vars=@./extra_vars.yaml

-$(KDEVOPS_NODES): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_NODES_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
+$(KDEVOPS_NODES): .config $(ANSIBLE_CFG_FILE) $(ANSIBLE_INVENTORY_FILE) $(KDEVOPS_NODES_TEMPLATE) $(KDEVOPS_EXTRA_VARS)

Target result:
make -j12 V=1
==> [sudo]
==> [sudo]
==> [make]
==> [make]
==> [nc]
==> [nc]
==> [ansible-playbook]
==> [ansible-playbook]
==> [scripts/version_check/ansible-playbook]
==> [scripts/version_check/ansible-playbook]
==> [ansible-requirements]
==> [/home/dagomez.linux/src/kdevops/ansible.cfg]
==> [timesyncd-server]
==> [journal-server]
==> [.kdevops.depcheck]
==> [.kdevops.depcheck]
==> [guestfs/kdevops_nodes.yaml]
==> [/home/dagomez.linux/src/kdevops/hosts]

> 
> Fixes: 5457b742d611 ("Makefile: fix target dependency order")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index fb7e9b816dbc..c1dd1232c7ff 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -258,7 +258,7 @@ ifneq (,$(KDEVOPS_BRING_UP_DEPS))
>  include scripts/bringup.Makefile
>  endif
>  
> -$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
> +$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_NODES) $(KDEVOPS_EXTRA_VARS)
>  	$(Q)ANSIBLE_LOCALHOST_WARNING=False ANSIBLE_INVENTORY_UNPARSED_WARNING=False \
>  		ansible-playbook $(ANSIBLE_VERBOSE) \
>  		$(KDEVOPS_PLAYBOOKS_DIR)/gen_hosts.yml \

Re: [RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Chuck Lever]

On 9/28/25 12:53 PM, Daniel Gomez wrote:
> I think both approaches work because nodes do not seem to be requiring hosts.
> The questions is whether we need both changes? I think we need to ensure 
> nodes are consistent with inventory changes. And they other way around in 
> case the user deletes any of the files. So, what do you think about adding
> this on top of the above?

I don't understand the Makefile well enough to have an informed opinion!
But I can try out both alternatives this afternoon.

-- 
Chuck Lever

Re: [RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Daniel Gomez]

On 28/09/2025 21.57, Chuck Lever wrote:
> On 9/28/25 12:53 PM, Daniel Gomez wrote:
>> I think both approaches work because nodes do not seem to be requiring hosts.
>> The questions is whether we need both changes? I think we need to ensure 
>> nodes are consistent with inventory changes. And they other way around in 
>> case the user deletes any of the files. So, what do you think about adding
>> this on top of the above?
> 
> I don't understand the Makefile well enough to have an informed opinion!
> But I can try out both alternatives this afternoon.
> 

No worries. I've tried all options myself and the sequence is the only thing
that changes between them. But I think is important to include nodes into
DEFAULT_DEPS after the provision path is triggered.

So, I'd go with my last suggestion which includes all changes.

Re: [RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Chuck Lever]

On 9/28/25 12:53 PM, Daniel Gomez wrote:
> I see! Thanks for reporting. Indeed my fix introduced this bug. I think
> it'd be best to move KDEVOPS_NODES just after we assign the filename to it
> (provision.Makefile -> guestfs.Makefile):
> 
> diff --git a/Makefile b/Makefile
> index 3653591d..26615b22 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -144,16 +144,16 @@ endif
>  DEFAULT_DEPS += $(ANSIBLE_CFG_FILE)
>  DEFAULT_DEPS += $(ANSIBLE_INVENTORY_FILE)
> 
> -ifneq (,$(KDEVOPS_NODES))
> -DEFAULT_DEPS += $(KDEVOPS_NODES)
> -endif
> -
>  include scripts/provision.Makefile
>  include scripts/firstconfig.Makefile
>  include scripts/systemd-timesync.Makefile
>  include scripts/journal-server.Makefile
>  include scripts/update_etc_hosts.Makefile
> 
> +ifneq (,$(KDEVOPS_NODES))
> +DEFAULT_DEPS += $(KDEVOPS_NODES)
> +endif
> +
>  KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_BRING_UP_DEPS_EARLY)
>  KDEVOPS_BRING_UP_DEPS += $(KDEVOPS_PROVISIONED_DEVCONFIG)

The above snippet seems to work without any new issues.


> @@ -264,13 +264,13 @@ ifneq (,$(KDEVOPS_BRING_UP_DEPS))
>  include scripts/bringup.Makefile
>  endif
> 
> -$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
> +$(ANSIBLE_INVENTORY_FILE): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_HOSTS_TEMPLATE) $(KDEVOPS_NODES) $(KDEVOPS_EXTRA_VARS)
>         $(Q)ansible-playbook $(ANSIBLE_VERBOSE) --connection=local \
>                 --inventory localhost, \
>                 $(KDEVOPS_PLAYBOOKS_DIR)/gen_hosts.yml \
>                 --extra-vars=@./extra_vars.yaml
> 
> -$(KDEVOPS_NODES): .config $(ANSIBLE_CFG_FILE) $(KDEVOPS_NODES_TEMPLATE) $(KDEVOPS_EXTRA_VARS)
> +$(KDEVOPS_NODES): .config $(ANSIBLE_CFG_FILE) $(ANSIBLE_INVENTORY_FILE) $(KDEVOPS_NODES_TEMPLATE) $(KDEVOPS_EXTRA_VARS)

With this snippet, "make" emits a new complaint:

TASK [ansible_cfg : Update ansible.cfg access modification time so make
sees it updated]
***************************************************************************************************************************************************
changed: [localhost]

PLAY RECAP
*********************************************************************************************************************************************************************************************************************************
localhost                  : ok=4    changed=2    unreachable=0
failed=0    skipped=0    rescued=0    ignored=0

make: Circular terraform/aws/nodes.tf <-
/home/cel/src/kdevops/buildbot-configs/hosts dependency dropped.

PLAY [Generate target node configuration]


But otherwise, "make bringup" works as expected.


-- 
Chuck Lever

Re: [RFC PATCH] Makefile: terraform wasn't building the kdevops_nodes file

[Daniel Gomez]

On 28/09/2025 22.30, Chuck Lever wrote:
> On 9/28/25 12:53 PM, Daniel Gomez wrote:
> With this snippet, "make" emits a new complaint:
> make: Circular terraform/aws/nodes.tf <-
> /home/cel/src/kdevops/buildbot-configs/hosts dependency dropped.

I see! We need to remove one or the other. My understanding is that the first
one (ANSIBLE_INVENTORY_FILE) may change the dependency to place nodes before the
inventory. The second (KDEVOPS_NODE) should keep the inventory before nodes.

So, both option should work but not both at once as I suggested yesterday.