-
Notifications
You must be signed in to change notification settings - Fork 14
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: fetch dhcp leases from network-manager via dbus #427
base: main
Are you sure you want to change the base?
Conversation
so the bootstrapURL in agent will now be a array of strings right?
|
yep, and then run/daemon will loop of this array |
I ran this and this is what I get
especially that previous line says:
while this
log from real hardware:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code fails in real life
7d190b1
to
2999446
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #427 +/- ##
==========================================
- Coverage 66.92% 61.93% -4.99%
==========================================
Files 17 20 +3
Lines 780 867 +87
==========================================
+ Hits 522 537 +15
- Misses 227 295 +68
- Partials 31 35 +4 ☔ View full report in Codecov by Sentry. |
sztp-agent/pkg/dhcp/dhcp_lease.go
Outdated
break | ||
} | ||
} | ||
return ExtractfromLine(line, `(?m)[^"]*`, 1), nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above, instead of return
let's append to a list and return the list at the end...
docker-compose.yml
Outdated
<<: *agent | ||
volumes: | ||
- client-certs:/certs | ||
- dhcp-leases-folder:/var/lib/dhclient/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this agent6 we only test network manager, right ?
then why we map dhcp-leases-folder
? I think should remove it, no ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea will remove it
sztp-agent/NetworkManager.conf
Outdated
@@ -0,0 +1,9 @@ | |||
[main] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this config is needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, not needed
Will remove and push
But yea I'm not able to get the bootstrap url via the network manager dbus @glimchb do you have any idea, am I doing anything wrong? |
if you take this code
and run it like this:
it works... so I suggest to start like this with this simple approach... and then the difference is loop over all active connections... vs single connection... try to see if you can start with working example and add tiny bits and see when it breaks.. don;t work with sztp code base, just create a small standalone |
sztp-agent/pkg/dhcp/dhcp_lease.go
Outdated
@@ -0,0 +1,33 @@ | |||
/* | |||
SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better file name lease.go
@@ -0,0 +1,73 @@ | |||
/* | |||
SPDX-License-Identifier: Apache-2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better file name dbus.go
@bhoopesh369 can we split into 2 PRs?
it will be easier for me to review and merge first PR asap... and then we can work on second one together... |
sztp-agent/pkg/dhcp/utils.go
Outdated
) | ||
|
||
// DHCPTestContent is a test content for DHCP Lease file | ||
const DHCPTestContent = `lease { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is not in the test.go file ?
sztp-agent/pkg/dhcp/utils.go
Outdated
} | ||
|
||
// DeleteTempTestFile deletes a temporary file | ||
func DeleteTempTestFile(file string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is not in the test.go file ?
sztp-agent/pkg/dhcp/utils.go
Outdated
} | ||
|
||
// CreateTempTestFile creates a temporary file with the given content | ||
func CreateTempTestFile(file string, content string, _ bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this is not in the test.go file ?
@bhoopesh369 please check #438 . |
I think when the client container starts, it runs dhclient -d -v, which sends DHCP discover messages over the opi network which is managed by docker. I think the connection is not managed by the network manager but rather by Docker’s internal networking. Am I wrong here ? Wdyt |
Yea works ig |
you are talking about CI |
Yep, I get that But like I have my environment set up in docker. Is there like a readme file for me to set this up in local? |
sure, https://github.com/opiproject/sztp/blob/main/scripts/run_agent.sh |
@glimchb The docker run commands I ran were:
I created a dummy dhcp0 interface via nmcli. but the options 143 was not present even though the conf file had them defined. |
@glimchb I had messaged you in slack 2 days before but didn't receive any response |
i'm traveling, will read soon |
69d5484
to
7a86f1d
Compare
Signed-off-by: Bhoopesh <[email protected]>
Signed-off-by: Bhoopesh <[email protected]>
bdd3fb0
to
3539961
Compare
Signed-off-by: Bhoopesh <[email protected]>
3539961
to
04725fa
Compare
I am not sure how to test this out since, currently, we have the lease file not managed by the network manager
so the agent-6 errors out that sztp_url don't exist,
I need to make the DHCP client connect to the DHCP server via network manager
I'm not sure how to do this.
Thoughts?
closes #418