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

RFC-0030 - Add support to Diego for file based service bindings #942

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
branch = main
[submodule "src/code.cloudfoundry.org/bbs"]
path = src/code.cloudfoundry.org/bbs
url = https://github.com/cloudfoundry/bbs
branch = main
url = https://github.com/dimitardimitrov13/bbs
branch = CFAR-929_VCAP_SERVICES
[submodule "src/code.cloudfoundry.org/auctioneer"]
Copy link
Member

Choose a reason for hiding this comment

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

This looks to me like an unwanted change here

Copy link
Author

Choose a reason for hiding this comment

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

Yes, you're right. I need to run the Diego Release test suite first. After it passes, the modules will be reverted.

path = src/code.cloudfoundry.org/auctioneer
url = https://github.com/cloudfoundry/auctioneer
Expand Down
1 change: 1 addition & 0 deletions jobs/rep/spec
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ templates:
tls.key.erb: config/certs/tls.key
tls_ca.crt.erb: config/certs/tls_ca.crt
indicators.yml.erb: config/indicators.yml
volume_mounted_files.erb: bin/volume_mounted_files

packages:
- pid_utils
Expand Down
2 changes: 2 additions & 0 deletions jobs/rep/templates/bpm-pre-start.erb
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,5 @@ bin_dir=/var/vcap/jobs/rep/bin
$bin_dir/set-rep-kernel-params

$bin_dir/setup_mounted_data_dirs

$bin_dir/volume_mounted_files
2 changes: 2 additions & 0 deletions jobs/rep/templates/rep.json.erb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
trusted_certs_dir = "/var/vcap/data/rep/shared/garden/trusted_certs"
instance_identity_dir = "/var/vcap/data/rep/shared/garden/instance_identity"
download_cache_dir= "/var/vcap/data/rep/shared/garden/download_cache"
volume_mounted_files = "/var/vcap/data/rep/shared/volume_mounted_files"

zone = spec.az
if_p("diego.rep.zone") do |value|
Expand Down Expand Up @@ -104,6 +105,7 @@
report_interval: "1m",
max_data_string_length: p("logging.max_data_string_length"),
max_log_lines_per_second: p("diego.executor.max_log_lines_per_second"),
volume_mounted_files: "#{volume_mounted_files}"
}

if p("containers.graceful_shutdown_interval_in_seconds") < 10
Expand Down
1 change: 1 addition & 0 deletions jobs/rep/templates/rep_ctl.erb
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ case $1 in

$bin_dir/set-rep-kernel-params
$bin_dir/setup_mounted_data_dirs
$bin_dir/volume_mounted_files

# Allowed number of open file descriptors
ulimit -n 100000
Expand Down
40 changes: 40 additions & 0 deletions jobs/rep/templates/volume_mounted_files.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#!/usr/bin/env bash

<%
_max_containers = 250
if_p("diego.rep.max_containers") do |value|
_max_containers = value
end
if _max_containers <= 0
raise "The max_containers prop should be a positive integer"
end
%>
max_containers=<%= _max_containers %>

# Define the service binding root directory
volume_mounted_files="/var/vcap/data/rep/shared/garden/volume_mounted_files"

# Calculate the size for the tmpfs (1MB per container)
Copy link
Member

Choose a reason for hiding this comment

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

This is going to increase the size of the diego footprint on the cell, which will reduce space for the containers themselves.

❓ Is additional space taken into account when the rep reports the resources on the cell to the BBS? It should be taken into account when this property is set to "auto".

Copy link
Author

Choose a reason for hiding this comment

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

Hello, that's new for me. Would you elaborate further on this? Thanks.

Copy link
Author

Choose a reason for hiding this comment

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

From my understanding, this https://github.com/cloudfoundry/executor/blob/main/initializer/configuration/configuration.go#L115 should be extended to add number_of_containers * 1MB to account for the 'overhead' that 'volume_mounted_files' will bring. Am I correct?

Copy link
Member

Choose a reason for hiding this comment

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

Yes! That looks correct to me.

root_tmpfs_size=$((max_containers * 1))M

# Ensure the root directory is safely removed and recreated
if [ -d "$volume_mounted_files" ]; then
# Ensure the root directory is unmounted before removing it
if mountpoint -q "$volume_mounted_files"; then
fuser -k "$volume_mounted_files"
umount -l "$volume_mounted_files"
fi

sleep 10

rm -rf "$volume_mounted_files"
fi

mkdir -p "$volume_mounted_files"

# Mount the root tmpfs
mount -t tmpfs -o size="$root_tmpfs_size" tmpfs "$volume_mounted_files" || exit 1

# Set permissions and ownership for the root directory and all subdirectories
chmod 0700 "$volume_mounted_files"
chown vcap:vcap "$volume_mounted_files"
15 changes: 14 additions & 1 deletion spec/rep_template_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,18 @@
end.to raise_error(/The max_containers prop should be a positive integer/)
end
end
end
end

describe 'volume_mounted_files.erb' do
let(:template) { job.template('bin/volume_mounted_files') }

context 'checks the max_containers value' do
it 'raises an error if max_containers is <= 0' do
deployment_manifest_fragment['diego']['rep']['max_containers'] = -10
expect do
rendered_template
end.to raise_error(/The max_containers prop should be a positive integer/)
end
end
end
ameowlia marked this conversation as resolved.
Show resolved Hide resolved
end