Host-Agent process not existing when Installer initialization failed

[dharmjit]

What steps did you take and what happened: Any errors returned by Installer.New are only logged in main.go but the process should also exit.

E0419 05:52:10.009133   15591 checks.go:38]  “msg”=“Failed pre-requisite packages precheck” “error”=“required package(s): [socat conntrack] not found”
E0419 05:52:10.009238   15591 main.go:206]  “msg”=“failed to instantiate installer” “error”=“precheck failed”

and later process failed with 

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x50 pc=0xca0fe7]

What did you expect to happen: Host-Agent process should exit when the Installer initialization failed.

[VibhorChinda]

Hey hii @dharmjit . I was going through this issue and would like to work on this.

What I understood is that : This funtion returns the error whenever it encounters it. That error gets logged in the main.go file

But apart from logging, the process should also exit.

Which process are you exactly talking about here ?? Like I see that New function returns / doesn't proceed further execution whenever it encounters any error.

[dharmjit]

I mean to exit the host-agent process i.e. return if error in Installater initialization.

[VibhorChinda]

ok Idk if my assumption is right or not.

we just need to write return here if error is encountered :|| Is it that simple ??

Idk I am assuming this only because what I see now is error is just logged here and process continues. we just need to return whenever we encounter error. Means just a return statement after this line

[anusha94]

we just need to write return

Yup. Sounds about right to me. What would be good though is to have an integration test for this.

[VibhorChinda]

Okay sounds interesting. Can someone assign this to me. Will work on this :))

[anusha94]

/assign @VibhorChinda

[anusha94]

The test can go in this file - host-agent-test.go

Here are some notes on testing tools and techniques that we use / follow - https://github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/blob/main/CONTRIBUTING.md#writing-tests

[VibhorChinda]

The test can go in this file - host-agent-test.go

Here are some notes on testing tools and techniques that we use / follow - https://github.com/vmware-tanzu/cluster-api-provider-bringyourownhost/blob/main/CONTRIBUTING.md#writing-tests

Ummm @anusha94 I was wondering do we really need to write test for this ??

Like have a look here it returns condition false when error is not nil i.e whenver we encounter error while installing process.

[anusha94]

do we really need to write test

Most of the times, the answer is yes :)

This is a very important workflow. I know its a single line fix. But it has repercussions if it remains untested. The idea is to have installer return nil if something goes wrong in initialization - mostly pre-checks failing. Pre-checks are something that should be detected early on in the lifecycle of the agent. Like you rightly pointed, it returns false in some condition later on - and then we have to revert the state etc. All of this adds to the complexity and rolling back may not be cheap. These checks were introduced to fail fast if something is not right. Hope that answers your question. Happy to debate further as to why we need a test for this 😄

[VibhorChinda]

Hey @anusha94 thanks for such long reply. Now I know more why such tests are important for projects and why they should be written.

But the main thing where I am stuck is what test should I write which validates my changes. Like all I could have thought about was to write an else condition for this if block and return nil or condition false.

Which is already there. So I could not think of any other way of testing it. Some help will be good :)) Thanks.

[VibhorChinda]

@sachinkumarsingh092 whenever you're free. Can you help me here too please. Thanks

[dharmjit]

Apologies for responding late here, One test case that I could think of is "should return when installer initialization fails due to empty downloadpath arg"

[VibhorChinda]

Thanks @dharmjit for giving some headway. Will try to implement this :))