Skip to content

Conversation

@phot0n
Copy link
Member

@phot0n phot0n commented Jan 6, 2026

addresses: #4386

agent: frappe/agent#412

need to implement the nat failover (maybe automatic?) - will do it in a separate pr - have the base for it by using a secondary private ip in this pr itself

@codecov
Copy link

codecov bot commented Jan 6, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 108 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.74%. Comparing base (f734da3) to head (d9fce0c).
⚠️ Report is 30 commits behind head on develop.

Files with missing lines Patch % Lines
...s/press/doctype/virtual_machine/virtual_machine.py 34.48% 38 Missing ⚠️
press/press/doctype/nat_server/nat_server.py 0.00% 26 Missing ⚠️
press/press/doctype/server/server.py 40.00% 15 Missing ⚠️
press/api/monitoring.py 0.00% 14 Missing ⚠️
...ess/press/doctype/proxy_failover/proxy_failover.py 11.11% 8 Missing ⚠️
press/agent.py 70.00% 6 Missing ⚠️
press/press/doctype/site/site.py 0.00% 1 Missing ⚠️

❌ Your patch check has failed because the patch coverage (33.33%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4402      +/-   ##
===========================================
- Coverage    50.81%   50.74%   -0.07%     
===========================================
  Files          846      848       +2     
  Lines        67670    67842     +172     
  Branches       284      284              
===========================================
+ Hits         34387    34428      +41     
- Misses       33255    33386     +131     
  Partials        28       28              
Flag Coverage Δ
dashboard 72.89% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@phot0n
Copy link
Member Author

phot0n commented Jan 7, 2026

bruh - prolly in tests we dont set public ips? will have to have a proper look

@phot0n phot0n force-pushed the remove-pub-ip branch 2 times, most recently from f157290 to 17100b6 Compare January 22, 2026 18:45
@phot0n phot0n changed the title feat: remove public ip for app and db servers by default feat: remove public ip for app and db servers Jan 22, 2026
@phot0n phot0n force-pushed the remove-pub-ip branch 4 times, most recently from e5e848a to ab948ca Compare January 22, 2026 22:02
@phot0n
Copy link
Member Author

phot0n commented Jan 22, 2026

wth did i break lol

phot0n added 18 commits January 23, 2026 13:09
* also consider the end server's private ip
* also enable the field if no other proxy in the cluster has it enabled
…trollers

* also add method for provisioning secondary private ip (for nat server)
* also cleanup the bastion logic by using any proxy server in the cluster
* also consider app and db servers with only private ip for monitoring using proxy server
@phot0n phot0n marked this pull request as ready for review January 23, 2026 09:52
Comment on lines +1219 to +1221
ids = ",".join(agent_job_ids) if isinstance(agent_job_ids, (list, tuple)) else agent_job_ids
return self.get(f"agent-jobs/{ids}")
Copy link
Member Author

Choose a reason for hiding this comment

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

dunno why this started failing just now

"Type": "A",
"TTL": 3600 if self.doctype == "Proxy Server" else 300,
"ResourceRecords": [{"Value": self.ip}],
"ResourceRecords": [{"Value": self.ip or self.private_ip}],
Copy link
Member Author

Choose a reason for hiding this comment

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

could be problematic?

"Proxy Server",
{"status": "Active", "cluster": cluster, "use_as_proxy_for_agent_and_metrics": 1},
)

Copy link
Member Author

@phot0n phot0n Jan 19, 2026

Choose a reason for hiding this comment

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

should we error out here if we dont get a proxy?

server_ip, server_private_ip = frappe.db.get_value(
self.server_type, self.server, ("ip", "private_ip")
)
if not server_ip and server_private_ip and not frappe.flags.in_test:
Copy link
Member Author

Choose a reason for hiding this comment

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

i dont like that i have to resort to in_test flag for the tests to pass

but in our tests i think we by default dont set any public ip

Copy link
Member Author

Choose a reason for hiding this comment

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

should i fix the tests? or should i rely on this flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe i should have another flag?

Comment on lines +27 to +44
- name: Create Netplan configuration for secondary IP
copy:
dest: /etc/netplan/60-secondary-ip.yaml
mode: '0644'
content: |
network:
dhcp4: true
ethernets:
{{ network_interface.stdout }}:
dhcp4: true
addresses:
- {{ primary_ip }}/16
- {{ secondary_ip }}/16
when: secondary_ip is defined and secondary_ip

- name: Apply Netplan
command: netplan apply
when: secondary_ip is defined and secondary_ip
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

we can also do sudo ip addr add {{secondary_ip}}/16 dev eth0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant