Skip to content
This repository has been archived by the owner on Nov 13, 2018. It is now read-only.

Always insert a 'volume=x' parameter into storage.config #1255

Merged
merged 3 commits into from
Apr 5, 2016
Merged

Always insert a 'volume=x' parameter into storage.config #1255

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b07cbac
Always insert a 'volume=x' parameter into storage.config
jschmerge Mar 31, 2016
15fdc8c
Address a few concerns about the default mixing up volume defaults
jschmerge Apr 5, 2016
4024ab9
More edge cases addressed.
jschmerge Apr 5, 2016
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
31 changes: 23 additions & 8 deletions traffic_ops/app/lib/UI/ConfigFiles.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -815,15 +815,12 @@ sub storage_dot_config_volume_text {
my $prefix = shift;
my $letters = shift;
my $volume = shift;
my $has_multiple_volumes = shift;

my $text = "";
my @postfix = split( /,/, $letters );
foreach my $l ( sort @postfix ) {
$text .= $prefix . $l;
if ($has_multiple_volumes) {
$text .= " volume=" . $volume;
}
$text .= " volume=" . $volume;
$text .= "\n";
}
return $text;
Expand All @@ -838,24 +835,42 @@ sub storage_dot_config {
my $text = $self->header_comment( $server->host_name );
my $data = $self->param_data( $server, $file );

my $has_multiple_volumes = get_num_volumes($data) > 1;
# always default to volume one and let DB params override
my $assigned_volume = 1;

if ( defined( $data->{Drive_Prefix} ) ) {
if ( defined($data->{Disk_Volume} ) ) {
$assigned_volume = $data->{Disk_Volume};
}
$text .= storage_dot_config_volume_text(
$data->{Drive_Prefix}, $data->{Drive_Letters},
$data->{Disk_Volume}, $has_multiple_volumes
$assigned_volume
);
Copy link
Member

Choose a reason for hiding this comment

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

Should we increment $assigned_volume here, and after we call storage_dot_config_volume_text below as well? If we don't, and $data->{RAM_Volume} or $data->{SSD_Volume} aren't set but their _Drive_Prefix is, they'll wrongly get the previously set volume.

}

# Default to 2 here
$assigned_volume = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if you have a RAM_Drive_Prefix parameter in the profile but no RAM_Volume parameter, and the Disk_Volume parameter is set to 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Shouldn't the UI prevend that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should and does are two separate things. These particular configuration items are just parameters in the parameter table that are mapped to a profile. That means there's zero field validation happening; the wild wild west. Anything is possible; they could set RAM_Volume to X if they wanted to.


if ( defined( $data->{RAM_Drive_Prefix} ) ) {
if ( defined($data->{RAM_Volume} ) ) {
$assigned_volume = $data->{RAM_Volume};
}
$text .= storage_dot_config_volume_text(
$data->{RAM_Drive_Prefix}, $data->{RAM_Drive_Letters},
$data->{RAM_Volume}, $has_multiple_volumes
$assigned_volume
);
}

# ...and default to 3 here
$assigned_volume = 3;

if ( defined( $data->{SSD_Drive_Prefix} ) ) {
if ( defined($data->{SSD_Volume} ) ) {
$assigned_volume = $data->{SSD_Volume};
}
$text .= storage_dot_config_volume_text(
$data->{SSD_Drive_Prefix}, $data->{SSD_Drive_Letters},
$data->{SSD_Volume}, $has_multiple_volumes
$assigned_volume
);
}
return $text;
Expand Down